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

Trott · 2019-04-16T13:48:59Z

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 · 2019-04-16T13:49:01Z

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

Trott · 2019-04-16T13:53:10Z

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 · 2019-04-16T15:28:08Z

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

Trott · 2019-04-16T15:29:06Z

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

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.

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 · 2019-04-16T15:29:47Z

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

Fixed and force-pushed.

benchmark/buffers/buffer-base64-decode.js

BridgeAR · 2019-04-16T15:52:11Z

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 · 2019-04-16T15:56:34Z

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

Trott · 2019-04-16T16:01:44Z

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 · 2019-04-16T16:06:50Z

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 · 2019-04-16T18:33:47Z

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

benchmark/buffers/buffer-base64-decode.js

nodejs-github-bot · 2019-04-16T20:05:55Z

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

Trott · 2019-04-17T13:47:20Z

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!

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Trott · 2019-04-18T17:26:43Z

Landed in f98679f...d5bb500

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 requested a review from BridgeAR April 16, 2019 13:49

Trott changed the title ~~buffer: fix buffer-base64--decode.js~~buffer: fix buffer-base64-decode.jsApr 16, 2019

Trott force-pushed the fix-buffer-benchmark branch from 46d2dfc to 3db71caCompare April 16, 2019 13:50

addaleax approved these changes Apr 16, 2019
View reviewed changes

jasnell approved these changes Apr 16, 2019
View reviewed changes

Trott added 2 commits April 16, 2019 08:29

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.

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 force-pushed the fix-buffer-benchmark branch from 2d01e7d to eab7d50Compare April 16, 2019 15:29

Trott changed the title ~~buffer: fix buffer-base64-decode.js~~benchmark: fix buffer-base64-decode.jsApr 16, 2019

BridgeAR reviewed Apr 16, 2019
View reviewed changes

benchmark/buffers/buffer-base64-decode.js Outdated Show resolvedHide resolved

fixup! benchmark: fix buffer-base64-decode.js
b72b012

BridgeAR approved these changes Apr 16, 2019
View reviewed changes

benchmark/buffers/buffer-base64-decode.js Outdated Show resolvedHide resolved

lpinca approved these changes Apr 16, 2019
View reviewed changes

fixup! fixup! benchmark: fix buffer-base64-decode.js
d371b34

addaleax approved these changes Apr 16, 2019
View reviewed changes

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 added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 17, 2019

refack approved these changes Apr 18, 2019
View reviewed changes

Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 18, 2019

Trott closed this Apr 18, 2019

Trott deleted the fix-buffer-benchmark branch January 13, 2022 22:51

Uh oh!

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

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

Uh oh!

Conversation

Trott commented Apr 16, 2019

Checklist

Uh oh!

nodejs-github-bot commented Apr 16, 2019

Uh oh!

Trott commented Apr 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 16, 2019

Uh oh!

Trott commented Apr 16, 2019

Uh oh!

Trott commented Apr 16, 2019

Uh oh!

Uh oh!

BridgeAR commented Apr 16, 2019

Uh oh!

nodejs-github-bot commented Apr 16, 2019

Uh oh!

Trott commented Apr 16, 2019

Uh oh!

BridgeAR commented Apr 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 16, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 16, 2019

Uh oh!

Trott commented Apr 17, 2019

Uh oh!

Trott commented Apr 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Trott commented Apr 16, 2019•
edited
Loading

BridgeAR commented Apr 16, 2019•
edited
Loading