-
Notifications
You must be signed in to change notification settings - Fork 30k
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 speedups #1048
Buffer speedups #1048
Conversation
https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/239/nodes=iojs-armv7-ubuntu1401/ https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/234/nodes=iojs-armv7-ubuntu1401/ takes longer on there, 12 mins vs 11 mins - note this machine is using ccache so builds are super fast. |
'size: 0x' + kMaxLength.toString(16) + ' bytes'); | ||
// Slightly less common case. | ||
if (typeof(arg) === 'string') { | ||
fromString(this, arg, arguments.length > 1 ? arguments[1] : 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do a length check here just to check if encoding is a string in fromString()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two reasons, one aesthetic, one technical. Aesthetic: it matches the check at the top of the function. Technical: an unguarded arguments[1]
is megamorphic whereas the length check keeps the property/index lookups monomorphic.
I didn't benchmark it to the death but I did look at the generated code and came to the conclusion that monomorphic is best, even with the additional property lookup. I don't know if you have looked at the machine code for KeyedLoadIC_Megamorphic but it's pretty complex.
My perf results on x86_64 Linux:
Minor perf improvement and code is more readable. Just have a couple questions, but LGTM. |
237c67f
to
79facc3
Compare
@trevnorris Updated, PTAL. @rvagg I'm not sure if I would trust the CI for anything related to benchmarking. |
@bnoordhuis "reliable" CI benchmarking is planned down the road |
too bad about needing to use |
The Buffer constructor is used pervasively throughout io.js, yet it was one of the most unwieldy functions in core. This commit breaks up the constructor into several small functions in a way that makes V8 happy. About 8-10% CPU time was attributed to the constructor function before in buffer-heavy benchmarks. That pretty much drops to zero now because V8 can now easily inline it at the call site. It shortens the running time of the following simple benchmark by about 15%: for (var i = 0; i < 25e6; ++i) new Buffer(1); And about 8% from this benchmark: for (var i = 0; i < 1e7; ++i) new Buffer('x', 'ucs2'); PR-URL: nodejs#1048 Reviewed-By: Trevor Norris <[email protected]>
Avoid a costly String#toLowerCase() call in Buffer#write() in the common case, i.e., that the string is already lowercase. Reduces the running time of the following benchmark by about 40%: for (var b = Buffer(1), i = 0; i < 25e6; ++i) b.write('x', 'ucs2'); PR-URL: nodejs#1048 Reviewed-By: Trevor Norris <[email protected]>
79facc3
to
4ddd640
Compare
R=@trevnorris?
With these patches,
make test
finishes (for the first time ever!) in under two minutes on my MBA. It shaves off about 5 seconds from the total running time. :-)