-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
buffer: various performance improvements #12361
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
PR-URL: nodejs#12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in 46f2026...4749ec2 |
Not sure if it's too late to do anything, but I think this should have technically been Before: > Buffer.from('foo','ajdkl')
TypeError: "encoding" must be a valid string encoding
... After: > Buffer.from('foo','ajdkl')
TypeError: Unknown encoding: ajdkl
... The code that produces the first error message above is preserved, but it is moved to a location where it will never execute. Unfortunately, this wasn't caught by tests because the only test case we have for that error (in assert.throws(() => Buffer.from('', 'buffer'), TypeError); |
@Trott I think that can be easily fixed without affecting anything performance-wise. |
This should no longer be semver-major when landed with #12439. |
I'm dropping semver-major now that the regression fix has landed, in case we should want to land/backport these two PRs in other branches in the future ... |
PR-URL: #12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #12361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mscdex should this be backported? If not please add the |
@MylesBorins It would probably need some benchmarking first to be sure it's still faster in previous V8 versions. |
Should land with #12439. |
I'm opting to not land this as we are getting close to maintenance if we change our mind we need to also include #14131 |
Various performance improvements for a few different
buffer
methods. Here are some results:I've decided not to mark this as semver-major despite changes such as
.toString()
now returning early whenbuf.length === 0
, in which case it won't validate the encoding name. The reason being there are already similar short-circuits being done in that method depending on the values ofstart
andend
.As far as I know there shouldn't be any other semver-major-like changes.
CI: https://ci.nodejs.org/job/node-test-pull-request/7352/
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)