★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/27260Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@refack refack

@jasnell jasnell

@addaleax addaleax

@lpinca lpinca

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready benchmark buffer
Projects
None yet
Milestone
No milestone
8 participants
@Trott @nodejs-github-bot @mscdex @BridgeAR @refack @jasnell @addaleax @lpinca
@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.

Sign in to view
Copy link

commented Apr 16, 2019

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

@nodejs-github-bot nodejs-github-bot added benchmark buffer labels Apr 16, 2019

@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.

Sign in to view
Copy link
Member Author

commented Apr 16, 2019 •
edited

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).
@addaleax

addaleax approved these changes Apr 16, 2019
View changes

@jasnell

jasnell approved these changes Apr 16, 2019
View changes

@mscdex

This comment has been minimized.

Sign in to view
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.

Sign in to view
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

@Trott
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.
3f8ac4a
@Trott
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.
Loading status checks…
eab7d50

@Trott Trott force-pushed the Trott:fix-buffer-benchmark branch from 2d01e7d to eab7d50 Apr 16, 2019

@Trott

This comment has been minimized.

Sign in to view
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

BridgeAR reviewed Apr 16, 2019
View changes

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

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link

commented Apr 16, 2019

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

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link
Member

commented Apr 16, 2019 •
edited

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.

Sign in to view
Copy link

commented Apr 16, 2019

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

BridgeAR approved these changes Apr 16, 2019
View changes

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

lpinca approved these changes Apr 16, 2019
View changes

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 16, 2019

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

addaleax approved these changes Apr 16, 2019
View changes

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

@Trott

This comment has been minimized.

Sign in to view
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
View changes

@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

@Trott
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>
3973354

Trott added a commit to Trott/io.js that referenced this pull request Apr 18, 2019

@Trott
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>
d5bb500
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 18, 2019

Landed in f98679f...d5bb500

This was referenced Apr 23, 2019

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Open
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.