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.

Sign up for GitHub

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

Jump to bottom

Improve benchmarks accuracy #148

Merged
merged 10 commits into from
Nov 27, 2016
Merged

Improve benchmarks accuracy #148

merged 10 commits into from
Nov 27, 2016

Conversation

kangax
Copy link
Member

@kangax kangax commented Sep 5, 2016 •
edited
Loading

This changes benchmark suite to run tests multiple times and get average. It also introduces "best size" and "best speed" versions of babili.

kangax
kangax reviewed Sep 5, 2016
View reviewed changes
package.json
"babel-plugin-transform-minify-booleans": "^6.8.0",
"babel-plugin-transform-property-literals": "^6.8.0",
"babel-plugin-transform-simplify-comparison-operators": "^6.8.0",
"babel-plugin-transform-undefined-to-void": "^6.8.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we can avoid this? Needed for benchmarks to be able to modify plugin options (such as simplify).

@kangax
Copy link
Member Author

kangax commented Nov 25, 2016

@boopathi now that preset options are in, I guess let me change PR to use that

kangax
kangax commented Nov 25, 2016
View reviewed changes
Copy link
Member Author

@kangax kangax left a comment

Choose a reason for hiding this comment

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

Inline

scripts/benchmark.js
presets: [
[require("../packages/babel-preset-babili"), {
simplify: {
multiPass: true
Copy link
Member Author

Choose a reason for hiding this comment

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

@boopathi this doesn't seem to show any difference in any of the files I'm testing...

Copy link
Member

Choose a reason for hiding this comment

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

there is no multipass option implemented in simplify.

Copy link
Member

Choose a reason for hiding this comment

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

You implemented it here - in this commit - 827a835#diff-05684bffcab704dd5d3842adbf921357 and your latest changes don't seem to have this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I'm an idiot, was thinking that we already added it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It still doesn't seem to work though. Am I missing something else? Should I change anything in options-manager?

boopathi
boopathi reviewed Nov 26, 2016
View reviewed changes
packages/babel-plugin-minify-simplify/src/index.js
if (!multiPass) {
return;
}

earlyReturnTransform(path);

if (!path.node[shouldRevisit]) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't multipass affect this ?

It throws an error for me - Container is falsy. I suppose this happens when calling path.replaceWith on the path whose node is modified.

Copy link
Member

Choose a reason for hiding this comment

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

I tried the script from the readme -

./scripts/benchmark.js react@0.14.3 react/dist/react.js

@boopathi
Copy link
Member

boopathi commented Nov 26, 2016 •
edited
Loading

Also, the run time metric looks inverted ?

                    raw     raw win gzip    gzip win parse time run time (average)
babili (best size)  34.36kB 262%    12.03kB 147%     2ms        698ms
babili (best speed) 34.37kB 262%    12.03kB 147%     2ms        947ms

@kangax
Copy link
Member Author

kangax commented Nov 27, 2016

@boopathi I just remembered that you removed Function: { enter: ... } (https://github.com/babel/babili/blob/4c4c10f42c8071c8b9ade0b92bb31b566008e204/packages/babel-plugin-minify-simplify/src/index.js) so that's probably why there's no difference now?

@boopathi
Copy link
Member

Ah. yeah. #282

@kangax
Copy link
Member Author

kangax commented Nov 27, 2016

How about I remove multiPass for now, and merge the rest? That way we can pass options through presets in a benchmark since we'll likely have them in a future.

@boopathi
Copy link
Member

Cool! This means we run each benchmark 3 times .. right?

@kangax
Copy link
Member Author

kangax commented Nov 27, 2016

Yep. We can also expose it via command line flag ./scripts/benchmark.js ... --runs=10

@boopathi boopathi added the Tag: Internal Pull Request changing project internals - code that is NOT published label Nov 27, 2016
@boopathi boopathi merged commit ffca8e6 into master Nov 27, 2016
@boopathi boopathi deleted the better_benchmarks branch November 27, 2016 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boopathi boopathi boopathi left review comments

Assignees
No one assigned
Labels
Tag: Internal Pull Request changing project internals - code that is NOT published
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kangax @boopathi