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

Use Uint8Array instead of Buffer in Node; faster string writing #59

Merged
merged 3 commits into from
Aug 25, 2016

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 25, 2016

  • Got rid of the buffer shim; now both Node and browsers use Uint8Array under the hood for consistency and simpler code (closing Replace buffer implementation with standard Uint8Array #35)
  • Made string writing slightly faster — it now uses the same technique as writeMessage (reserving 1 byte for length and then shifting data if it's more than 1 instead of encoding into a temporary array of bytes)
  • Fixed the array copying behavior when decoding in the browser
  • Decoding benchmark performance dropped by ~11% in Node, but I think this is fine
  • Encoding benchmark performance increased by ~14% in Node
  • Browser benchmark results are the same

cc @jfirebaugh @kjvalencik

@jfirebaugh
Copy link

Excellent!

@mourner mourner merged commit d00b6aa into master Aug 25, 2016
@mourner mourner deleted the uint8array branch August 25, 2016 22:08
@kjvalencik
Copy link
Collaborator

IMHO, this is a pretty substantial change because anything that is expecting a Buffer may either break or convert to a buffer which is a copy operation.

In your experience, is copying large-ish chunks of memory like this a big performance cost?

@tmcw
Copy link
Contributor

tmcw commented Aug 29, 2016

This'll be a 3.x, right, so dependents wouldn't automatically upgrade.

@mourner
Copy link
Member Author

mourner commented Aug 29, 2016

@kjvalencik it's a breaking change, so it prompts a semver-major release (along with some more subtle changes after v2.0.1). It's easy to adapt to though — you can do either Buffer.from(arr) (copy) or Buffer.from(arr.buffer) (shares memory) if you depend specifically on the Buffer API (which is already implemented internally as Uint8Array as of Node 4 AFAIR).

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.

None yet

4 participants