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

Backport 7157 to v6.x (buffer: speed up swap16/32, add swap64) #7546

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 5, 2016

Straightforward conflict resolution for #7157

CI because it’s possible: https://ci.nodejs.org/job/node-test-commit/3981/

* Speed up buffer.swap16 and swap32 by using builtins. Up to ~6x gain.
  Drop transition point between JS and C++ implementations accordingly.
  Amount of performance improvement not only depends on buffer size but
  also memory alignment.
* Fix tests: C++ impl tests were testing 0-filled buffers so were
  always passing.
* Add similar buffer.swap64 method.
* Make buffer-swap benchmark mirror JS impl.

doc/api/buffer.markdown has an entry of "added: REPLACEME" that should
be changed to the correct release number before tagged.

Because node is currently using a very old version of cpplint.py it
doesn't know that std::swap() has moved from <algorithm> to <utility> in
c++11. So until cpplint.py is updated simply NOLINT the line.
Technically it should be NOLINT(build/include_what_you_use), but that
puts the line over 80 characters causing another lint error.

PR-URL: nodejs#7157
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. v6.x labels Jul 5, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 5, 2016
@Fishrock123
Copy link
Contributor

Note to self: land f8d3f6f after this

@jasnell
Copy link
Member

jasnell commented Jul 5, 2016

LGTM

@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@addaleax
Copy link
Member Author

addaleax commented Jul 5, 2016

The CI run is green-ish except for the usual hiccups, including completely failed ARM builds… nothing related, anyway, this should be good to go.

@Fishrock123
Copy link
Contributor

I'll land it on v6.x after I land the current bunch of commits in #7441

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
* Speed up buffer.swap16 and swap32 by using builtins. Up to ~6x gain.
  Drop transition point between JS and C++ implementations accordingly.
  Amount of performance improvement not only depends on buffer size but
  also memory alignment.
* Fix tests: C++ impl tests were testing 0-filled buffers so were
  always passing.
* Add similar buffer.swap64 method.
* Make buffer-swap benchmark mirror JS impl.

doc/api/buffer.markdown has an entry of "added: REPLACEME" that should
be changed to the correct release number before tagged.

Because node is currently using a very old version of cpplint.py it
doesn't know that std::swap() has moved from <algorithm> to <utility> in
c++11. So until cpplint.py is updated simply NOLINT the line.
Technically it should be NOLINT(build/include_what_you_use), but that
puts the line over 80 characters causing another lint error.

PR-URL: #7157
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-URL: #7546
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 5, 2016

Thanks! landed in 4014ecb

@Fishrock123 Fishrock123 closed this Jul 5, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 5, 2016

Also landed 3cba8ac afterwards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants