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

benchmark: Add remaining path benchmarks & optimize #2103

Closed
wants to merge 1 commit into from
Closed

benchmark: Add remaining path benchmarks & optimize #2103

wants to merge 1 commit into from

Conversation

nwoltman
Copy link
Contributor

@nwoltman nwoltman commented Jul 4, 2015

As a follow-up to 0d15161, this commit adds benchmarks for the rest of the path functions and also forces V8 to optimize the functions before starting the benchmark test (as suggested in a comment by @evanlucas).

@mscdex mscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Jul 4, 2015
@evanlucas
Copy link
Contributor

Should we maybe force optimize like in https://github.com/nodejs/io.js/blob/master/benchmark/url/url-parse.js#L30?

@nwoltman
Copy link
Contributor Author

nwoltman commented Jul 5, 2015

That sounds like a good idea to me.

@nwoltman nwoltman changed the title benchmark: Add remaining path benchmarks benchmark: Add remaining path benchmarks & optimize Jul 5, 2015
p.basename('/foo/bar/baz/asdf/quux.html');
v8.setFlagsFromString('--allow_natives_syntax');
eval('%OptimizeFunctionOnNextCall(p.basename)');
p.basename('/foo/bar/baz/asdf/quux.html');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the call is made in all the cases before benchmark is started?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because it takes V8 extra time to optimize the function when it is called after %OptimizeFunctionOnNextCall is called on it. So calling the function before the benchmark starts means that the time it takes for V8 to optimize the function does not get recorded as part of the benchmark time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice :-) Did you check if v8 is really going to optimize the function? If the function is never going to be optimized by v8, then we don't have to use this, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it doesn't now, that could change in the future, so it might be worth keeping it there anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mscdex Oh okay then :-)

@nwoltman nwoltman changed the title benchmark: Add remaining path benchmarks & optimize benchmark: add remaining path benchmarks & optimize Jul 11, 2015
@nwoltman nwoltman changed the title benchmark: add remaining path benchmarks & optimize benchmark: Add remaining path benchmarks & optimize Jul 11, 2015
@brendanashworth
Copy link
Contributor

LGTM

As a follow-up to 0d15161, this commit adds benchmarks for the rest
of the path functions and also forces V8 to optimize the functions
before starting the benchmark test.
brendanashworth pushed a commit that referenced this pull request Jul 27, 2015
As a follow-up to 0d15161, this commit adds benchmarks for the rest
of the path functions and also forces V8 to optimize the functions
before starting the benchmark test.

PR-URL: #2103
Reviewed-By: Brendan Ashworth <[email protected]>
@brendanashworth
Copy link
Contributor

landed in 99d9d7e, thanks!

@nwoltman nwoltman deleted the path-bench branch July 31, 2015 21:27
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants