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: remove es benchmark #22361

Closed
wants to merge 1 commit into from

Conversation

psmarshall
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

These benchmarks compare the performance of ES6 features and their ES5 equivalents. These are testing the javascript engine itself and not node in any way, as far as I can tell. six-speed does a good job testing these things already, and there is a lot of overlap between these two suites. We also have alerts set up for six-speed on the V8 side so we will see any regressions before they make it into node. I'd suggest removing these benchmarks entirely.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. labels Aug 16, 2018
@jasnell
Copy link
Member

jasnell commented Aug 16, 2018

I'm generally -1 on this but won't block. I use these frequently to test improvements in performance across v8 updates.

@mscdex
Copy link
Contributor

mscdex commented Aug 16, 2018

I agree with @jasnell that these benchmarks can still be useful to us.

@Trott
Copy link
Member

Trott commented Aug 16, 2018

@BridgeAR
Copy link
Member

@jasnell @mscdex If one of these cases is not covered by six-speed, it should be possible to move them over.

Since neither of you is blocking, I still mark this as author-ready.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 17, 2018
@Trott
Copy link
Member

Trott commented Aug 18, 2018

@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

@psmarshall
Copy link
Contributor Author

@jasnell If you are using this frequently then I guess it is worth keeping - I'd argue these types of benchmarks are better suited to external suites like six-speed but I guess in your case, the benefit is having the performance runner within node and getting the numbers out in a format that is useful to you.

@psmarshall
Copy link
Contributor Author

I'm going to close this given that it is in use. We could revisit it once:

  1. We know if there is anything in this suite that is not covered by six-speed
  2. There is an easy way to run six-speed for different node versions and compare them

@psmarshall psmarshall closed this Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants