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

buffer: remove Uint8Array check #11324

Merged
merged 1 commit into from Feb 16, 2017

Conversation

Projects
None yet
@seishun
Member

seishun commented Feb 12, 2017

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

@seishun seishun changed the title from buffer: remove Uit8Array check to buffer: remove UInt8Array check Feb 12, 2017

@seishun seishun changed the title from buffer: remove UInt8Array check to buffer: remove Uint8Array check Feb 12, 2017

buffer: remove Uint8Array check
This makes write[U]Int* operations on Buffer with `noAssert=false` about 3
times faster.

Refs: #11245
@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 12, 2017

Contributor

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

Contributor

thefourtheye commented Feb 12, 2017

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

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Feb 12, 2017

Member

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

Member

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.

@@ -1056,8 +1056,6 @@ Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) {
function checkInt(buffer, value, offset, ext, max, min) {
if (!isUint8Array(buffer))

This comment has been minimized.

@joyeecheung

joyeecheung Feb 12, 2017

Member

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

@joyeecheung

joyeecheung Feb 12, 2017

Member

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 12, 2017

Member

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.

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

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 12, 2017

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.

Member

addaleax commented Feb 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Feb 12, 2017

Member

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?

Member

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?

@targos

targos approved these changes Feb 12, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 12, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 13, 2017

Contributor

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

Contributor

thefourtheye commented Feb 13, 2017

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

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Feb 13, 2017

Member

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

Member

seishun commented Feb 13, 2017

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

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 13, 2017

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.

Contributor

thefourtheye commented Feb 13, 2017

@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

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

@seishun seishun merged commit 00c86cc into nodejs:master Feb 16, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 16, 2017

Member

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

Member

gibfahn commented Feb 16, 2017

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Feb 16, 2017

Member

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

Member

MylesBorins commented Feb 16, 2017

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

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Feb 25, 2017

Member

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

Member

italoacasas commented Feb 25, 2017

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 26, 2017

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Feb 28, 2017

Member

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

Member

italoacasas commented Feb 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex
Contributor

mscdex commented Feb 28, 2017

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 7, 2017

Member

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

Member

evanlucas commented Mar 7, 2017

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

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 17, 2017

Member

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.

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 seishun:remove-uint8array-check branch Jun 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment