buffer: remove Uint8Array check#11324
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the buffer 
seishun changed the title 
This makes write[U]Int* operations on Buffer with `noAssert=false` about 3 times faster. Refs: #11245
thefourtheye commented Feb 12, 2017
This would be a major change, as it changes the error message. |
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. |
| functioncheckInt(buffer,value,offset,ext,max,min){ | ||
| if(!isUint8Array(buffer)) |
There was a problem hiding this comment.
If buffer is no longer checked we can just pass buffer.length down. Not sure about the performance implication though.
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 left a comment
jasnell left a comment There was a problem hiding this comment.
The change itself LGTM but I'd like discussion about the semver-iness of this before it lands.
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 commented Feb 12, 2017
Could you describe some of them in more detail? |
jasnell commented Feb 12, 2017
thefourtheye commented Feb 13, 2017
Just to be sure, we can have a CITGM run. |
seishun commented Feb 13, 2017
@thefourtheye Could you run it? Last time I tried it, everything broke. |
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 left a comment
trevnorris left a comment There was a problem hiding this comment.
Personally I don't think this needs to be semver-major.
gibfahn commented Feb 16, 2017
@nodejs/citgm this citgm run crashed and burned, do we know what's going on here? |
MylesBorins commented Feb 16, 2017
@seishun why was this landed with so many failures on citgm that had not yet been reviewed? |
italoacasas commented Feb 25, 2017
@MylesBorins do you have the time to review the failures in CITGM? |
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/ |
A lot of red in CITGM, the error in expressjs seems the root issue.
|
mscdex commented Feb 28, 2017
evanlucas commented Mar 7, 2017
This isn't cherry-picking cleanly to v7.x-staging. Want to backport? |
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 |
13 participants
This makes write[U]Int* operations on Buffer with
noAssert=falseabout 3 times faster.See #11245 (comment) for background and benchmark results.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
buffer