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

buffer: remove Uint8Array check #11324

Merged
merged 1 commit into from
Feb 16, 2017
Merged

buffer: remove Uint8Array check #11324

merged 1 commit into from
Feb 16, 2017

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Feb 12, 2017 •
edited
Loading

This makes write[U]Int* operations on Buffer with noAssert=false about 3 times faster.

See #11245 (comment) for background and benchmark results.

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

buffer

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 12, 2017
@seishun seishun changed the title buffer: remove Uit8Array check buffer: remove UInt8Array check Feb 12, 2017
@seishun seishun changed the title buffer: remove UInt8Array check buffer: remove Uint8Array check Feb 12, 2017
@seishun
buffer: remove Uint8Array check
603e2c8 
This makes write[U]Int* operations on Buffer with `noAssert=false` about 3
times faster.

Refs: #11245
@thefourtheye
Copy link
Contributor

This would be a major change, as it changes the error message.

@seishun
Copy link
Contributor Author

seishun commented Feb 12, 2017

@thefourtheye It removes the error message. I don't think this should be semver-major, it's semver-minor at most. I find it hard to imagine code that relies on this exception.

joyeecheung
joyeecheung reviewed Feb 12, 2017
View reviewed changes
lib/buffer.js
@@ -1056,8 +1056,6 @@ Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) {


function checkInt(buffer, value, offset, ext, max, min) {
if (!isUint8Array(buffer))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If buffer is no longer checked we can just pass buffer.length down. Not sure about the performance implication though.

@jasnell
Copy link
Member

jasnell commented Feb 12, 2017

There have been cases in the past where removal of an error has caused things to break. I'm going to agree with semver-major on this but we can see how the rest of @nodejs/ctc feels.

jasnell
jasnell approved these changes Feb 12, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself LGTM but I'd like discussion about the semver-iness of this before it lands.

@addaleax
Copy link
Member

I don’t think any usage that would have triggered the error can reasonably be considered part of our API, so I don’t think this a change that needs a semver label.

@seishun
Copy link
Contributor Author

seishun commented Feb 12, 2017

There have been cases in the past where removal of an error has caused things to break.

Could you describe some of them in more detail?

@jasnell
Copy link
Member

jasnell commented Feb 12, 2017

@seishun ... one in particular that is not related to this module is glob, which uses an error thrown by fs.realpath() to determine when to exit. I believe @addaleax is correct in that general usage should not trigger this so semver-patch may be just fine, we just need to be sure.

@thefourtheye
Copy link
Contributor

Just to be sure, we can have a CITGM run.

@seishun
Copy link
Contributor Author

seishun commented Feb 13, 2017

@thefourtheye Could you run it? Last time I tried it, everything broke.

@thefourtheye
Copy link
Contributor

@seishun Started. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/573/. I hasn't succeeded in the recent past. Let's see.

trevnorris
trevnorris approved these changes Feb 15, 2017
View reviewed changes
Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't think this needs to be semver-major.

@seishun seishun merged commit 00c86cc into nodejs:master Feb 16, 2017
@gibfahn
Copy link
Member

gibfahn commented Feb 16, 2017

@nodejs/citgm this citgm run crashed and burned, do we know what's going on here?

@MylesBorins
Copy link
Contributor

@seishun why was this landed with so many failures on citgm that had not yet been reviewed?

@seishun seishun mentioned this pull request Feb 23, 2017
Closed
@italoacasas
Copy link
Contributor

@MylesBorins do you have the time to review the failures in CITGM?

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2017

CITGM again since the previous link is dead already: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/603/

@italoacasas
Copy link
Contributor

italoacasas commented Feb 28, 2017 •
edited
Loading

A lot of red in CITGM, the error in expressjs seems the root issue.

Uncaught TypeError: The "digest" argument is required and must not be undefined

cc: @mscdex @MylesBorins

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2017

@italoacasas See: expressjs/express#3207

@evanlucas
Copy link
Contributor

This isn't cherry-picking cleanly to v7.x-staging. Want to backport?

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported to LTS? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@seishun seishun deleted the remove-uint8array-check branch June 17, 2017 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@trevnorris trevnorris trevnorris approved these changes

Assignees
No one assigned
Labels
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.