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: fix buffer-base64-decode.js #27260

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
8 participants
@Trott
Copy link
Member

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

This comment has been minimized.

@Trott Trott requested a review from BridgeAR Apr 16, 2019

@Trott Trott changed the title buffer: fix buffer-base64--decode.js buffer: fix buffer-base64-decode.js Apr 16, 2019

@Trott Trott force-pushed the Trott:fix-buffer-benchmark branch from 46d2dfc to 3db71ca Apr 16, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

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

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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

Trott added some commits Apr 16, 2019

benchmark: fix buffer-base64-decode.js
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.
test: fix test-benchmark-buffer
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 Trott force-pushed the Trott:fix-buffer-benchmark branch from 2d01e7d to eab7d50 Apr 16, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

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

This comment has been minimized.

Show resolved Hide resolved benchmark/buffers/buffer-base64-decode.js Outdated
@lpinca

lpinca approved these changes Apr 16, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott Trott added the author ready label Apr 16, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

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 label Apr 17, 2019

@refack

refack approved these changes Apr 18, 2019

@Trott Trott removed the fast-track 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

benchmark: fix buffer-base64-decode.js
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

test: fix test-benchmark-buffer
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

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Landed in f98679f...d5bb500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.