Skip to content

benchmark: Add remaining path benchmarks & optimize - #2103

Closed
nwoltman wants to merge 1 commit into
nodejs:masterfrom
nwoltman:path-bench
Closed

benchmark: Add remaining path benchmarks & optimize#2103
nwoltman wants to merge 1 commit into
nodejs:masterfrom
nwoltman:path-bench

Conversation

@nwoltman

Copy link
Copy Markdown
Contributor

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).

@mscdexmscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Jul 4, 2015
@evanlucas

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

That sounds like a good idea to me.

@nwoltmannwoltman changed the title benchmark: Add remaining path benchmarksbenchmark: Add remaining path benchmarks & optimizeJul 5, 2015

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 :-)

@nwoltmannwoltman changed the title benchmark: Add remaining path benchmarks & optimizebenchmark: add remaining path benchmarks & optimizeJul 11, 2015
@nwoltmannwoltman changed the title benchmark: add remaining path benchmarks & optimizebenchmark: Add remaining path benchmarks & optimizeJul 11, 2015
@brendanashworth

Copy link
Copy Markdown
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 <brendan.ashworth@me.com>
@brendanashworth

Copy link
Copy Markdown
Contributor

landed in 99d9d7e, thanks!

@cjihrigcjihrig mentioned this pull request Jul 28, 2015
@nwoltman
nwoltman deleted the path-bench branch July 31, 2015 21:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@nwoltman@evanlucas@brendanashworth@mscdex@thefourtheye