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 fixes #9064

Closed
wants to merge 3 commits into from
Closed

Benchmark fixes #9064

wants to merge 3 commits into from

Conversation

AndreasMadsen
Copy link
Member

@AndreasMadsen AndreasMadsen commented Oct 12, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

Description of change

Some minor benchmark fixes. I can create 3 separate pull requests if that makes the review process easier.

benchmark: fixes csv parsing given no parameters

When a benchmark did not contain any parameters the csv configuration
filed would be "". In R this is by default parsed as NA, causing NA in
the printout too.

Fixes: #9061

benchmark: change the execution order

This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659

benchmark: use node v4 syntax in common.js

Using new syntax such as ...args means that the benchmark suite can't
be used with older node versions. This changes the ...args syntax to a
good old Array.prototype.slice.

Refs: #8932 (comment)

When a benchmark did not contain any parameters the csv configuration
filed would be "". In R this is by default parsed as NA, causing NA in
the printout too.

Fixes: #9061
This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659
Using new syntax such as `...args` means that the benchmark suite can't
be used with older node versions. This changes the `...args` syntax to a
good old `Array.prototype.slice`.

Refs: #8932 (comment)
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Oct 12, 2016
@AndreasMadsen
Copy link
Member Author

/cc @mscdex

@jasnell
Copy link
Member

jasnell commented Oct 14, 2016

Not overly keen on this. But technically LGTM

@mscdex
Copy link
Contributor

mscdex commented Oct 14, 2016

@jasnell Which part did you have reservations about?

I can confirm the csv parsing fix works.

@jasnell
Copy link
Member

jasnell commented Oct 14, 2016

The changes to v8ForceOptimization. It's no biggie tho.

@AndreasMadsen
Copy link
Member Author

@jasnell: @trevnorris touched a bit on it in #8637 (comment).

I'd like to point out that by keeping benchmarks using the absolute latest features it makes it impossible to test them against older versions of Node. Requiring editing the file and removing the offending syntax. I'd say for benchmarks that don't involve new JS features we use the backwards compatible syntax as much as possible.

@mscdex
Copy link
Contributor

mscdex commented Oct 14, 2016

LGTM

@jasnell jasnell self-assigned this Oct 17, 2016
jasnell pushed a commit that referenced this pull request Oct 17, 2016
When a benchmark did not contain any parameters the csv configuration
filed would be "". In R this is by default parsed as NA, causing NA in
the printout too.

Fixes: #9061
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
Using new syntax such as `...args` means that the benchmark suite can't
be used with older node versions. This changes the `...args` syntax to a
good old `Array.prototype.slice`.

Refs: #8932 (comment)
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 17, 2016

Landed in 138a792, 41173de, and 8a0b7ff

@jasnell jasnell closed this Oct 17, 2016
jasnell pushed a commit that referenced this pull request Oct 17, 2016
When a benchmark did not contain any parameters the csv configuration
filed would be "". In R this is by default parsed as NA, causing NA in
the printout too.

Fixes: #9061
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
Using new syntax such as `...args` means that the benchmark suite can't
be used with older node versions. This changes the `...args` syntax to a
good old `Array.prototype.slice`.

Refs: #8932 (comment)
PR-URL: #9064
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@AndreasMadsen
Copy link
Member Author

@jasnell thanks for merging this, kinda forgot about it :)

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
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.

benchmark: 'NA' appended to filename in compare.R output
4 participants