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

benchmark: fix buffer-base64-decode.js #27260

Closed
wants to merge 4 commits into from
Closed

benchmark: fix buffer-base64-decode.js #27260

wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 16, 2019

693401d added stricter range checking
for buffer operations and that apparently seems to have uncovered the
fact that one of our benchmarks was overflowing a buffer. Increase the
buffer size so the benchmark doesn't throw an error anymore.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3274

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. labels Apr 16, 2019
@Trott Trott requested a review from BridgeAR April 16, 2019 13:49
@Trott Trott changed the title buffer: fix buffer-base64--decode.js buffer: fix buffer-base64-decode.js Apr 16, 2019
@Trott
Copy link
Member Author

Trott commented Apr 16, 2019 •
edited
Loading

Still need a fix for test-benchmark-buffes which sets values such that buffer-base64-encode also fails. Will get to that later if no one beats me to it (which would be great).

@mscdex
Copy link
Contributor

mscdex commented Apr 16, 2019

I think this should be using the benchmark: prefix instead of buffer: in the commit message?

@Trott
Copy link
Member Author

Trott commented Apr 16, 2019

Fixed the test too with a simple one-line change. A quick re-review would be great. @addaleax @jasnell

@Trott
benchmark: fix buffer-base64-decode.js
3f8ac4a 
693401d added stricter range checking
for buffer operations and that apparently seems to have uncovered the
fact that one of our benchmarks was overflowing a buffer. Increase the
buffer size so the benchmark doesn't throw an error anymore.
@Trott
test: fix test-benchmark-buffer
eab7d50 
Using `len=2` in test-benchmark-buffer was resulting in a `RangeError`
in buffer-base64-encode.js. Change to `len=256` which works in all
buffer benchmarks.
@Trott
Copy link
Member Author

Trott commented Apr 16, 2019

I think this should be using the benchmark: prefix instead of buffer: in the commit message?

Fixed and force-pushed.

@Trott Trott changed the title buffer: fix buffer-base64-decode.js benchmark: fix buffer-base64-decode.js Apr 16, 2019
@BridgeAR
Copy link
Member

Still need a fix for test-benchmark-buffes which sets values such that buffer-base64-encode also fails.

Please add an assertion that the benchmark fails in case faulty len values are used. That way this would have been detected earlier as well.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22500/

@Trott
Copy link
Member Author

Trott commented Apr 16, 2019

Still need a fix for test-benchmark-buffes which sets values such that buffer-base64-encode also fails.

Please add an assertion that the benchmark fails in case faulty len values are used. That way this would have been detected earlier as well.

I'm not sure I understand. The benchmark test was failing. Adding an assertion for it wouldn't have detected it because it was already a failing test. (The problem is that the test is not run in CI except nightly, which is where i saw the failure.) The benchmark tests are intentionally minimal. I'm reluctant to test buffer functionality in them.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 16, 2019 •
edited
Loading

Adding an assertion for it wouldn't have detected it because it was already a failing test.

The test would have failed earlier that way and we have some very basic assertions in some benchmarks to validate the input parameters. Adding one here as well just seemed right to me but that's not a blocker (my other comment is).

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22511/

BridgeAR
BridgeAR approved these changes Apr 16, 2019
View reviewed changes
benchmark/buffers/buffer-base64-decode.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22513/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019
@Trott
Copy link
Member Author

Trott commented Apr 17, 2019

I'd like to fast-track this to unbreak the nightly CI that runs the benchmark tests. 👍 here to approve if you're a Collaborator, or leave a comment if you think it's just not that important and this should wait the remaining 24 hours. Thanks!

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 17, 2019
@Trott Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 18, 2019
@Trott Trott closed this Apr 18, 2019
Trott added a commit to Trott/io.js that referenced this pull request Apr 18, 2019
@Trott
benchmark: fix buffer-base64-decode.js
3973354 
693401d added stricter range checking
for buffer operations and that apparently seems to have uncovered the
fact that one of our benchmarks was overflowing a buffer. Increase the
buffer size so the benchmark doesn't throw an error anymore.

PR-URL: nodejs#27260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Apr 18, 2019
@Trott
test: fix test-benchmark-buffer
d5bb500 
Using `len=2` in test-benchmark-buffer was resulting in a `RangeError`
in buffer-base64-encode.js. Change to `len=256` which works in all
buffer benchmarks.

PR-URL: nodejs#27260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 18, 2019

Landed in f98679f...d5bb500

This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@Trott Trott deleted the fix-buffer-benchmark branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
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. buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Trott @nodejs-github-bot @mscdex @BridgeAR @refack @jasnell @addaleax @lpinca