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

Revert "Add PooledSlicedByteBuf and PooledDuplicatedByteBuf" #4138

Closed

Conversation

Scottmitch
Copy link
Member

Motivation:
Currently the "derived" buffer will only ever be recycled if the release call is made on the "derived" object, and the "wrapped" buffer ends up being "fully released" (aka refcount goes to 0). From my experience this is not the common use case and thus the "derived" buffers will not be recycled.

Modifications:

Result:
Less complexity, and less code to create new objects in majority of cases."

Motivation:
Currently the "derived" buffer will only ever be recycled if the release call is made on the "derived" object, and the "wrapped" buffer ends up being "fully released" (aka refcount goes to 0). From my experience this is not the common use case and thus the "derived" buffers will not be recycled.

Modifications:
- revert netty#3788

Result:
Less complexity, and less code to create new objects in majority of cases.
@Scottmitch Scottmitch force-pushed the revert_pooled_derived_recycle branch from da24f87 to a2bcd71 Compare August 26, 2015 20:19
@normanmaurer
Copy link
Member

+1

@Scottmitch
Copy link
Member Author

I will submit a followup PR to restore changes we want (reseting the indexes, etc..)

@Scottmitch
Copy link
Member Author

Cherry-picked 4.0 (33001e8) 4.1 (e280251) master (77ff24b)

Each commit was a git revert for the commits listed in #3788.

@Scottmitch Scottmitch closed this Aug 26, 2015
@Scottmitch Scottmitch deleted the revert_pooled_derived_recycle branch August 26, 2015 20:26
Scottmitch added a commit to Scottmitch/netty that referenced this pull request Aug 26, 2015
Motivation:
As part of the revert process in netty#4138 some index and mark updates were lost.

Modifications:
- Restore the index / mark updates made in netty#3788

Result:
Slice and Duplicate buffers index / marks are correctly initialized.
Scottmitch added a commit that referenced this pull request Aug 27, 2015
Motivation:
As part of the revert process in #4138 some index and mark updates were lost.

Modifications:
- Restore the index / mark updates made in #3788

Result:
Slice and Duplicate buffers index / marks are correctly initialized.
Scottmitch added a commit that referenced this pull request Aug 27, 2015
Motivation:
As part of the revert process in #4138 some index and mark updates were lost.

Modifications:
- Restore the index / mark updates made in #3788

Result:
Slice and Duplicate buffers index / marks are correctly initialized.
Scottmitch added a commit that referenced this pull request Aug 27, 2015
Motivation:
As part of the revert process in #4138 some index and mark updates were lost.

Modifications:
- Restore the index / mark updates made in #3788

Result:
Slice and Duplicate buffers index / marks are correctly initialized.
@ninja-
Copy link

ninja- commented Sep 4, 2015

@Scottmitch while using netty .30 I noticed that pooledslicedbytebuf isn't recycled properly anyway (frame decoder = more gc pressure). I understand your findings that it needed this uncommon .release() usage to recycle properly.
now that I see it removed for future netty version, are you planning something related to this so it could be recycled or something? (my alternative - using a .copy() instead)

https://github.com/Scottmitch/netty/blob/a2bcd713d4db88dd19bcb14d9865aa506acf9710/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java#L934 - an object would be created every time so what was your motivation here?

Maybe I should stop playing with these buffers and just deal with a single .copy() ...

@Scottmitch
Copy link
Member Author

@ninja- - .slice() and .duplicate() share the same underlying data (no copy) but just maintain different indexes over the data. copy() not only has difference indexes, but also does not share data (makes a copy). So they are different operations.

This PR fixes the complications introduced while attempting to "pool" the "derived" (slices, duplicates) taken from pooled buffers. However in most cases the pooling would never really happen and a new object would be allocated anyways because of the description of this PR.

Does that answer your question?

@ninja-
Copy link

ninja- commented Sep 4, 2015

@Scottmitch yes thanks for explaining. I am sticking with copy instead.

pulllock pushed a commit to pulllock/netty that referenced this pull request Oct 19, 2023
Motivation:
As part of the revert process in netty#4138 some index and mark updates were lost.

Modifications:
- Restore the index / mark updates made in netty#3788

Result:
Slice and Duplicate buffers index / marks are correctly initialized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants