Benchmark refactor #640

neeldug · 2020-08-16T17:50:26Z

This Pull Request fixes/closes #639 .

It changes the following:

Moves all test scripts to separate module in consts.rs
Removes all static definitions from various benchmarks

Potentially could move ALLOC to consts.rs, however, am unsure about this.

Avoids code repetition between exec, full, lexer & parser.

codecov · 2020-08-16T17:57:24Z

Codecov Report

Merging #640 into master will increase coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
+ Coverage   72.64%   72.92%   +0.27%     
==========================================
  Files         179      178       -1     
  Lines       13275    13230      -45     
==========================================
+ Hits         9644     9648       +4     
+ Misses       3631     3582      -49

Impacted Files	Coverage Δ
boa/src/builtins/object/gcobject.rs	`72.72% <0.00%> (-12.13%)`	⬇️
boa/src/exec/mod.rs	`71.06% <0.00%> (-3.01%)`	⬇️
boa/src/exec/tests.rs	`100.00% <0.00%> (ø)`
boa/src/exec/new/mod.rs	`75.00% <0.00%> (ø)`
boa/src/exec/block/mod.rs	`68.75% <0.00%> (ø)`
boa/src/builtins/map/tests.rs	`100.00% <0.00%> (ø)`
boa/src/builtins/regexp/mod.rs	`71.42% <0.00%> (ø)`
boa/src/exec/statement_list.rs	`68.75% <0.00%> (ø)`
boa/src/builtins/object/internal_state.rs
boa/src/environment/lexical_environment.rs	`75.49% <0.00%> (+0.73%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d025207...1d6a788. Read the comment docs.

Accidentally forgot to run cargo fmt. Fixed now!

neeldug · 2020-08-16T18:36:40Z

Can't quite work out why this is failing now, any help would be greatly appreciated!

HalidOdat · 2020-08-16T19:03:38Z

Can't quite work out why this is failing now, any help would be greatly appreciated!

Hi @neeldug. The problem is that benches/conts is interpreted as benchmark, every file like boa/examples/ is treated as a executable.

neeldug · 2020-08-16T19:05:23Z

Can't quite work out why this is failing now, any help would be greatly appreciated!

Hi @neeldug. The problem is that benches/conts is interpreted as benchmark, every file like boa/examples/ is treated as a executable.

Ahh I see, thanks, will try something else then.

Testing to see if benchmark runs here, can be moved elsewhere instead of src root.

Provides more descriptive location for bench scripts.

HalidOdat

The benchmarks shouldn't be exposed to the API, they should be isolated from it, instead of putting it in src/consts.rs we should make the benchmarks multi-file ones, like the examples ones:

boa:
 - benches:
   - exec
     - main.rs
     - consts.rs
   - full:
     - main.rs
     - consts.rs
...

neeldug · 2020-08-17T01:12:38Z

@HalidOdat

But wouldn't this result in the code repetition that was initially the goal to avoid? In having the consts being separated by type? Although I do agree, it shouldn't be exposed to the API.

Razican · 2020-08-17T07:03:18Z

Something that can be done is to have one file per each, and one constants.rs file at the same level, and include it with include!() at the top of each benchmark.

neeldug · 2020-08-17T17:26:43Z

Something that can be done is to have one file per each, and one constants.rs file at the same level, and include it with include!() at the top of each benchmark.

So would you suggest having each static variable to be defined in a separate file, then using the include macro in a separate constants.rs, and then include those in each of the benchmarking files?

Razican · 2020-08-17T20:37:50Z

Something that can be done is to have one file per each, and one constants.rs file at the same level, and include it with include!() at the top of each benchmark.

So would you suggest having each static variable to be defined in a separate file, then using the include macro in a separate constants.rs, and then include those in each of the benchmarking files?

We should try that, yes.

bench_scripts contains each static var definition, then is included into constants.rs, then imported into each benchmark.

🚀 Hopefully works 🚀

Works, but includes every constant regardless of whether all are needed, also can cause IDE linking issues.

Fixes #639. Remaining issues: include macro forces each script to import unneeded variables and IDE compatibility lost. Moved all bench scripts to consts.rs Avoids code repetition between exec, full, lexer & parser. Used multiline import. Accidentally forgot to run cargo fmt. Fixed now! Added pub keyword. Moving consts to src, subject to move Testing to see if benchmark runs here, can be moved elsewhere instead of src root. Moving consts to benches Provides more descriptive location for bench scripts. Forgot to add to lib Trying out include macro for less code repetition. bench_scripts contains each static var definition, then is included into constants.rs, then imported into each benchmark. Fixed rust fmt issues... 🚀 Hopefully works 🚀 Incorrect path for includes! Using include for each benchmark! Works, but includes every constant regardless of whether all are needed, also can cause IDE linking issues.

…nto benchmark-refactor

neeldug · 2020-08-17T22:54:17Z

Latest commit is fully functional, however, since it no longer uses use, IDE linking to definitions is lost, this is a minor issue. Although, it's also meaning that every variable is being imported into every benchmark, which also a minor issue.

EDIT: Works on local machine but strangely not CI, very strange behaviour.

Razican · 2020-08-18T13:37:51Z

Latest commit is fully functional, however, since it no longer uses use, IDE linking to definitions is lost, this is a minor issue. Although, it's also meaning that every variable is being imported into every benchmark, which also a minor issue.

I had thought of using one file for all, but what you did is even better, and it brings some interesting posibilities. We can use JavaScript (.js) files for each (instead of .rs), and use normal JavaScript syntax, then, we can use include_str!() to include those strings either in a constant or directly when passing the data to the lexer in each test.

What do you think?

EDIT: Works on local machine but strangely not CI, very strange behaviour.

The error is giving is indeed strange. Not sure if it's related (probably isn't), but I saw that you added a line between the Jemallocator static and its configuration. Could we remove that?

neeldug · 2020-08-18T14:45:42Z

I had thought of using one file for all, but what you did is even better, and it brings some interesting posibilities. We can use JavaScript (.js) files for each (instead of .rs), and use normal JavaScript syntax, then, we can use include_str!() to include those strings either in a constant or directly when passing the data to the lexer in each test.

What do you think?

Actually, this seems very reasonable, and in fact seems better than the previous implementation since, it means raw JavaScript benchmarks can be written without having to deal with any rust formatting. But for the include_str!() macro, does the macro format it as a string literal, i.e. removing the necessity for dealing with r#""#? Actually through testing, it does seem to remove the formatting necessity!

The error is giving is indeed strange. Not sure if it's related (probably isn't), but I saw that you added a line between the Jemallocator static and its configuration. Could we remove that?

Have removed the line, and will see on the next commit

Using include_str macro to import consts, perhaps could avoid having constants.rs and instead import separately?

neeldug · 2020-08-18T15:29:46Z

Strangely enough, same issue arises... Also numerous warnings spit out due to constants.rs being treated as a bench itself. Perhaps is solvable by putting constants.rs into the subdirectory as an index, and including it from there?

Razican · 2020-08-18T16:33:15Z

We don't need that file. We can just include the code samples directly on each benchmark.

neeldug · 2020-08-18T16:34:59Z

Ahh, okay. Should I add all the import_str's at the top or next to each call?

Static imports completed.

Razican · 2020-08-19T05:46:31Z

Ahh, okay. Should I add all the import_str's at the top or next to each call?

I think it's better at the call site, since we only use each once, right?

🚀 Moving static includes to adjacent to functions for better organisation. 🚀

Razican

This is good to go from my side :)

Lan2u

Looks great

neeldug added 3 commits August 16, 2020 17:02

Update

d5a755d

Merge branch 'master' of https://github.com/boa-dev/boa into master

baebd75

Moved all bench scripts to consts.rs

9f3fe05

Avoids code repetition between exec, full, lexer & parser.

neeldug added 2 commits August 16, 2020 19:12

Used multiline import.

2f0be91

Accidentally forgot to run cargo fmt. Fixed now!

Added pub keyword.

0619522

neeldug added 3 commits August 17, 2020 00:23

Moving consts to src, subject to move

bd77c74

Testing to see if benchmark runs here, can be moved elsewhere instead of src root.

Moving consts to benches

296f05a

Provides more descriptive location for bench scripts.

Forgot to add to lib

1b96fd1

neeldug marked this pull request as ready for review August 17, 2020 00:23

HalidOdat reviewed Aug 17, 2020

View reviewed changes

HalidOdat added benchmark Issues and PRs related to the benchmark subsystem. enhancement New feature or request labels Aug 17, 2020

neeldug added 7 commits August 17, 2020 22:55

Trying out include macro for less code repetition.

05d4e7b

bench_scripts contains each static var definition, then is included into constants.rs, then imported into each benchmark.

Fixed rust fmt issues...

c6f244d

🚀 Hopefully works 🚀

Incorrect path for includes!

ff54dd2

Using include for each benchmark!

c9b24ad

Works, but includes every constant regardless of whether all are needed, also can cause IDE linking issues.

Merge branch 'master' of https://github.com/boa-dev/boa into master

2e8329b

Merge branch 'benchmark-refactor' of https://github.com/neeldug/boa i…

c0a0e9e

…nto benchmark-refactor

neeldug requested a review from HalidOdat August 17, 2020 23:10

Moving rust scripts to javascript.

b32abf4

Using include_str macro to import consts, perhaps could avoid having constants.rs and instead import separately?

Moving all scripts to bench_scripts

00cc389

Static imports completed.

Razican added this to the v0.10.0 milestone Aug 19, 2020

Razican mentioned this pull request Aug 19, 2020

Added CLEAN_JS and MINI_JS benches #581

Merged

Moving static includes.

1d6a788

🚀 Moving static includes to adjacent to functions for better organisation. 🚀

Razican approved these changes Aug 19, 2020

View reviewed changes

Razican requested review from jasonwilliams and Lan2u August 19, 2020 12:52

Lan2u approved these changes Aug 19, 2020

View reviewed changes

Lan2u merged commit bbacf1d into boa-dev:master Aug 19, 2020

neeldug deleted the benchmark-refactor branch August 19, 2020 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Benchmark refactor #640

Benchmark refactor #640

neeldug commented Aug 16, 2020

codecov bot commented Aug 16, 2020 •

edited

Loading

neeldug commented Aug 16, 2020 •

edited

Loading

HalidOdat commented Aug 16, 2020

neeldug commented Aug 16, 2020

HalidOdat left a comment •

edited

Loading

neeldug commented Aug 17, 2020 •

edited

Loading

Razican commented Aug 17, 2020

neeldug commented Aug 17, 2020

Razican commented Aug 17, 2020

neeldug commented Aug 17, 2020 •

edited

Loading

Razican commented Aug 18, 2020

neeldug commented Aug 18, 2020 •

edited

Loading

neeldug commented Aug 18, 2020 •

edited

Loading

Razican commented Aug 18, 2020

neeldug commented Aug 18, 2020

Razican commented Aug 19, 2020

Razican left a comment

Lan2u left a comment

Benchmark refactor #640

Benchmark refactor #640

Conversation

neeldug commented Aug 16, 2020

codecov bot commented Aug 16, 2020 • edited Loading

Codecov Report

neeldug commented Aug 16, 2020 • edited Loading

HalidOdat commented Aug 16, 2020

neeldug commented Aug 16, 2020

HalidOdat left a comment • edited Loading

Choose a reason for hiding this comment

neeldug commented Aug 17, 2020 • edited Loading

Razican commented Aug 17, 2020

neeldug commented Aug 17, 2020

Razican commented Aug 17, 2020

neeldug commented Aug 17, 2020 • edited Loading

Razican commented Aug 18, 2020

neeldug commented Aug 18, 2020 • edited Loading

neeldug commented Aug 18, 2020 • edited Loading

Razican commented Aug 18, 2020

neeldug commented Aug 18, 2020

Razican commented Aug 19, 2020

Razican left a comment

Choose a reason for hiding this comment

Lan2u left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2020 •

edited

Loading

neeldug commented Aug 16, 2020 •

edited

Loading

HalidOdat left a comment •

edited

Loading

neeldug commented Aug 17, 2020 •

edited

Loading

neeldug commented Aug 17, 2020 •

edited

Loading

neeldug commented Aug 18, 2020 •

edited

Loading

neeldug commented Aug 18, 2020 •

edited

Loading