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.

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

Merged
merged 2 commits into from
Mar 5, 2015
Merged

Buffer speedups #1048

merged 2 commits into from
Mar 5, 2015

Conversation

bnoordhuis
Copy link
Member

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. :-)

@bnoordhuis
Copy link
Member Author

@rvagg
Copy link
Member

rvagg commented Mar 4, 2015

'size: 0x' + kMaxLength.toString(16) + ' bytes');
// Slightly less common case.
if (typeof(arg) === 'string') {
fromString(this, arg, arguments.length > 1 ? arguments[1] : 'utf8');
Copy link
Contributor

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()?

Copy link
Member Author

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.

@trevnorris
Copy link
Contributor

My perf results on x86_64 Linux:

230ns/op to 212ns/op for 0xff
456ns/op to 404ns/op for 'abc'
170ns/op to 157ns/op for [0x01, 0x02, 0x03]
209ns/op to 200ns/op for new Buffer('abc')

Minor perf improvement and code is more readable. Just have a couple questions, but LGTM.

@bnoordhuis
Copy link
Member Author

@trevnorris Updated, PTAL.

@rvagg I'm not sure if I would trust the CI for anything related to benchmarking.

@jbergstroem
Copy link
Member

@bnoordhuis "reliable" CI benchmarking is planned down the road

@trevnorris
Copy link
Contributor

too bad about needing to use n | 0 instead. LGTM.

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 <trev.norris@gmail.com>
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 <trev.norris@gmail.com>
@bnoordhuis bnoordhuis closed this Mar 5, 2015
@bnoordhuis bnoordhuis deleted the buffer-speedups branch March 5, 2015 18:45
@bnoordhuis bnoordhuis merged commit 4ddd640 into nodejs:v1.x Mar 5, 2015
@rvagg rvagg mentioned this pull request Mar 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants