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

Improve performance #654

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Improve performance #654

merged 2 commits into from
Apr 14, 2017

Conversation

crzidea
Copy link
Member

@crzidea crzidea commented Apr 13, 2017

Buffer#slice() in node is extremely slow and should avoid using it! Unfortunately, we didn't realize it when create this project. So, we should have a plan to improve the performance of this module:

  • Step 1: Using BufferList(bl) instead of raw Buffer to avoid calling Buffer#slice().
    I have a patch for this module, and tested it in production. In my case, I have reduced CPU usage from 70% to 50%, and memory usage from 400m to 200m, and increased throughput from 200k/min to 400k/min.
  • Step 2: Replace binary module. The module is 5 years old and have not be maintained for 5 years! kafka-node is heavily depended on binary but unfortunately binary used Buffer#slice(), a lot.

This patch complete Step 1, and I hope someone else could help me with the Step 2.

@crzidea crzidea requested a review from hyperlink April 13, 2017 09:54
@subzero10
Copy link

My opinion is that for this type of code you should write something yourself, which you know is fully optimized. This is protocol level and will not be changed often. However, BufferList looks quite good.

@hyperlink
Copy link
Collaborator

Awesome @crzidea! Looking forward to testing this!

Copy link
Collaborator

@hyperlink hyperlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a definite difference in performance.

Single consumer consuming 2.1 million messages:

30 partitions
before: 154s
after: 150s

1 partition
before: 194s
after: 189s

👏🏻

@crzidea
Copy link
Member Author

crzidea commented Apr 14, 2017

I will Merge this PR and start working on Step 2.

@crzidea crzidea merged commit 2ac4153 into SOHU-Co:master Apr 14, 2017
@crzidea
Copy link
Member Author

crzidea commented Apr 14, 2017

@hyperlink The difference in performance would be more convincing if you test on large messages.

hyperlink added a commit that referenced this pull request May 8, 2017
This reverts commit 2ac4153, reversing
changes made to 88abf37.
hyperlink added a commit that referenced this pull request May 8, 2017

* Revert "Check buffer length before processing (#657)"

This reverts commit 7753238.

* Revert "Merge pull request #654 from crzidea/bl"

This reverts commit 2ac4153, reversing
changes made to 88abf37.

* keep docker change
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.

3 participants