Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added CLEAN_JS and MINI_JS benches #581

Merged
merged 5 commits into from
Aug 20, 2020
Merged

Added CLEAN_JS and MINI_JS benches #581

merged 5 commits into from
Aug 20, 2020

Conversation

neeldug
Copy link
Contributor

@neeldug neeldug commented Jul 20, 2020

These benches are arbitrary code which is subject to change,
functionally the programs are identical.

This Pull Request fixes/closes #564 .

It changes the following:

  • Exec and Full benchmarks to run with minified and clean code for comparison.

Specifically went for slightly longer function for visible change when minifying code with whitespace and character savings.

These benches are arbitrary code which is subject to change,
functionally the programs are identical.
@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   71.25%   72.56%   +1.30%     
==========================================
  Files         176      179       +3     
  Lines       11557    13259    +1702     
==========================================
+ Hits         8235     9621    +1386     
- Misses       3322     3638     +316     
Impacted Files Coverage Δ
boa/src/builtins/object/internal_state.rs 0.00% <0.00%> (-41.67%) ⬇️
boa/src/builtins/value/rcstring.rs 69.23% <0.00%> (-7.70%) ⬇️
boa/src/builtins/number/mod.rs 62.28% <0.00%> (-6.16%) ⬇️
boa/src/builtins/error/mod.rs 32.00% <0.00%> (-4.37%) ⬇️
boa/src/builtins/object/mod.rs 38.11% <0.00%> (-4.16%) ⬇️
boa/src/builtins/value/conversions.rs 49.27% <0.00%> (-4.06%) ⬇️
boa/src/builtins/value/mod.rs 65.51% <0.00%> (-3.64%) ⬇️
boa/src/builtins/json/mod.rs 79.48% <0.00%> (-1.85%) ⬇️
boa/src/builtins/array/mod.rs 79.26% <0.00%> (-1.44%) ⬇️
boa/src/builtins/mod.rs 22.58% <0.00%> (-0.76%) ⬇️
... and 29 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 13f7374...9f623b1. Read the comment docs.

@neeldug neeldug marked this pull request as ready for review July 31, 2020 02:23
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really go in exec? this to me seems like we are benchmarking the lexer/parser and not execution.

What do you think? @Razican @jasonwilliams

@Razican Razican added the benchmark Issues and PRs related to the benchmark subsystem. label Aug 14, 2020
@Razican
Copy link
Member

Razican commented Aug 14, 2020

Should this really go in exec? this to me seems like we are benchmarking the lexer/parser and not execution.

If I'm not mistaken, the part inside of the bench() iterator is the execution (or the full thing), so they are in the correct place, but it would be very nice to add parsing benchmarks too.

@Razican Razican added the enhancement New feature or request label Aug 14, 2020
@Razican Razican added this to the v0.10.0 milestone Aug 14, 2020
@neeldug
Copy link
Contributor Author

neeldug commented Aug 16, 2020

Should this really go in exec? this to me seems like we are benchmarking the lexer/parser and not execution.

If I'm not mistaken, the part inside of the bench() iterator is the execution (or the full thing), so they are in the correct place, but it would be very nice to add parsing benchmarks too.

I can implement this in the same PR, if this is wanted?

@Razican
Copy link
Member

Razican commented Aug 16, 2020

I can implement this in the same PR, if this is wanted?

Yes, please, if you can, that would be more complete :)

Added identical benchmark cases for parser.
@Razican Razican added the blocked Waiting for another code change label Aug 19, 2020
@Razican
Copy link
Member

Razican commented Aug 19, 2020

I think this is blocked until #640 lands, since it will need a refactor.

@Lan2u
Copy link

Lan2u commented Aug 19, 2020

#640 is now merged so this is unblocked :)

@Lan2u Lan2u removed the blocked Waiting for another code change label Aug 19, 2020
Adding js scripts in bench_scripts & moving exec, full and parser
benchmarks to new include_str!() macro.
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good from my side, but let's wait for #646 to land in order to not fail the tests.

@Lan2u Lan2u merged commit 4009204 into boa-dev:master Aug 20, 2020
@neeldug neeldug deleted the mini-benchmarks branch August 20, 2020 11:46
Lan2u pushed a commit that referenced this pull request Aug 22, 2020
* Added CLEAN_JS and MINI_JS benches (#581)

* Added CLEAN_JS and MINI_JS benches

These benches are arbitrary code which is subject to change,
functionally the programs are identical.

* Forgot semicolon

* Adding parsing benchmarks for CleanJS & MiniJS

Added identical benchmark cases for parser.

* Migrating Clean and Mini benchmarks to new format

Adding js scripts in bench_scripts & moving exec, full and parser
benchmarks to new include_str!() macro.

* Move `require_object_coercible` to `Value` (#654)

Co-authored-by: neeldug <[email protected]>
Co-authored-by: HalidOdat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add benchmarks for "uglified" JS
5 participants