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

Incomplete buffer #13

Merged
merged 6 commits into from
Feb 8, 2015
Merged

Incomplete buffer #13

merged 6 commits into from
Feb 8, 2015

Conversation

shinichy
Copy link
Collaborator

@shinichy shinichy commented Feb 8, 2015

Hi, thank you for pointing out the performance issue. I found that bl.duplicate was the cause so I refactored decode method not to use it. I introduced internal tryDecode(buf, offset) method to keep track of current offset as you suggested. If everything goes well, tryDecode returns {value: , bytesConsumed: }, so decode (buf) method actually calls bl.consume(bytesConsumed) at last step. Otherwise, bl is intact.

I ran your benchmark with my new code again and it's as fast as master now!

new code

time 2726
decode/s 36683.78576669112

master

time 2706
decode/s 36954.91500369549

@mcollina
Copy link
Owner

mcollina commented Feb 8, 2015

This is impressive. GREAT!

mcollina added a commit that referenced this pull request Feb 8, 2015
@mcollina mcollina merged commit 1bc11d8 into master Feb 8, 2015
@mcollina mcollina deleted the incomplete_buffer branch February 8, 2015 09:47
@mcollina
Copy link
Owner

mcollina commented Feb 8, 2015

I would like to remove the header thing in the stream support, but we would need a major version bump.

What do you think? With this fix the header in the streams is not worth it.

@shinichy
Copy link
Collaborator Author

shinichy commented Feb 8, 2015

I agree. The header is not worth it anymore.

@mcollina
Copy link
Owner

mcollina commented Feb 8, 2015

I am quite full this week, would you like to send a PR for that too?

@shinichy
Copy link
Collaborator Author

shinichy commented Feb 9, 2015

OK. I'll work on it this weekend.

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.

2 participants