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 Buffer.allocUnsafe to reduce allocations and CPU usage #695

Merged
merged 2 commits into from
Aug 7, 2022

Conversation

chkimes
Copy link
Contributor

@chkimes chkimes commented Jul 13, 2022

Buffer.allocUnsafe will prefer to create non-zeroed buffers using the buffer pool rather than creating a new zeroed buffer each time like Buffer.alloc. Despite the name, Buffer.allocUnsafe is safe to use if you are immediately overwriting all the content in the buffer, and it is preferred for library APIs that will be called a lot or will be doing a lot of allocations.

https://nodejs.org/api/buffer.html#static-method-bufferallocunsafesize

@kibertoad
Copy link
Collaborator

@Uzlopak I summon thee, the master of Node.js performance

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 1, 2022

@kibertoad

Well yes it is better. We have just to make sure that we always completely write the Buffer.

@chkimes
Copy link
Contributor Author

chkimes commented Aug 1, 2022

At least in these two instances where the buffers are created, the lengths are known ahead of time and the new buffers are immediately filled with data.

@cressie176
Copy link
Collaborator

Seems reasonable to me. I think it's worth adding a comment above each usage of allocUnsafe linking to this PR to discourage someone getting unnecessarily concerned, or worse, accidentally replicating when the buffers are not immediately filled. Unless any objections I'll merge and publish this weekend.

@chkimes
Copy link
Contributor Author

chkimes commented Aug 4, 2022

I can update the PR with appropriate documentation. Linking this PR is probably unnecessary, since it should be already tied to the commit and findable for anyone who wants to review the surrounding context?

@cressie176
Copy link
Collaborator

I can update the PR with appropriate documentation. Linking this PR is probably unnecessary, since it should be already tied to the commit and findable for anyone who wants to review the surrounding context?

Thanks @chkimes, I see you've added a comment. A link to the PR is preferred because then people can easily find a whole discussion. I understand you could find this from the commit, but without a hint in the code it might easily be missed. I can easily sort after I merge though.

@cressie176 cressie176 merged commit 92a7cdd into amqp-node:main Aug 7, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 7, 2022

Btw. Good job

@cressie176
Copy link
Collaborator

I decided to keep the original comment, but added a link to the PR too. Publishing as amqplib@0v.10.2 shortly. Thank you @chkimes and @Uzlopak

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.

5 participants