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

Cumulation optimization #280

Merged
merged 5 commits into from Apr 24, 2012
Merged

Cumulation optimization #280

merged 5 commits into from Apr 24, 2012

Conversation

normanmaurer
Copy link
Member

This changes try to minimize the size of the cumulation buffer to a minimum. Before we used a DynamicChannelBuffer which was never "shrinked" again. The problem with the previous implementation was that it could lead to very big cumulation buffers.

For example if you receive a buffer with 65k it will create a cumulation buffer with min. 65k + whatsleft. If you are lucky enough you will even get a cumulation buffer of 2 * 65k as the DynamicChannelBuffer will double its size if it needs more room.

The changes here try to fix it.

@trustin please review

@ghost ghost assigned normanmaurer Apr 23, 2012
@normanmaurer
Copy link
Member Author

Seems like the speed and memory usage is really good..

See:
https://twitter.com/#!/timfox/status/194465361066860544

buf.writeBytes(input);
} else {
// wrap the cumulation and input
buf = ChannelBuffers.wrappedBuffer(cumulation, input);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't use a composite buffer - it's known to perform pretty bad and it will make hotspot confused because of one more ChannelBuffer implementation to consider.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also just use a HeapChannelBuffer but I'm not sure this would be good in terms of memory usage and speed (byte copies)

Copy link
Member

Choose a reason for hiding this comment

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

I realized that your change limits the number of components in the composite buffer to 2. Because the number of components are very small, I think it might be OK.

@normanmaurer
Copy link
Member Author

@trustin so ok to pull in ?

@trustin
Copy link
Member

trustin commented Apr 24, 2012

Yes, once buffer factory issues are fixed.

@trustin
Copy link
Member

trustin commented Apr 24, 2012

Ah, could we compare the performance between the composite buffer version and the heap buffer version?

@normanmaurer
Copy link
Member Author

Let me try to write one but Tim's perf tests are promising with CompositeChannelBuffer

@trustin
Copy link
Member

trustin commented Apr 24, 2012

Yeah, I just wonder if it's gonna be even faster with the heap buffer. Could you ask Time for the numbers? :-)

@trustin
Copy link
Member

trustin commented Apr 24, 2012

We'll have to rewrite this part when we switch to a heap buffer.

+                // create a new buffer and copy the readable buffer into it
+                this.cumulation = newCumulationBuffer(ctx, buf.readableBytes());
+                this.cumulation.writeBytes(buf);

@normanmaurer
Copy link
Member Author

Why?

@normanmaurer
Copy link
Member Author

@trustin Tim tried his benchmark also with HeapChannelBuffer but he saw no speed improvements. So I think we should merge it like it is

normanmaurer added a commit that referenced this pull request Apr 24, 2012
Make the cumulation usage more memory efficient
@normanmaurer normanmaurer merged commit b4c00f0 into 3 Apr 24, 2012
@trustin
Copy link
Member

trustin commented Apr 25, 2012

I see. Thanks!

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

2 participants