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

Fix misaligned buffer allocation method (fixes #1367) #1400

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

dmandalidis
Copy link
Contributor

No description provided.

@mp911de
Copy link
Collaborator

mp911de commented Aug 31, 2020

Care to elaborate? Multiplying a float with an int value uses float multiplication for both values. After the multiplication, the value gets downcasted to int again.

It seems we need to differentiate whether we use ByteBufUtil.writeUtf8(…) for writing or our own method. ByteBufUtil requires a bit more memory for writing whereas the current implementation tries to optimize for memory.

@mp911de mp911de added status: waiting-for-feedback We need additional information before we can continue type: bug A general bug and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 31, 2020
@dmandalidis
Copy link
Contributor Author

@mp911de I think that the problem is that both Lettuce and Netty are allocating buffers in a different way. This fix makes StringCodec aligned with the Netty way, but it seems to be a workaround in general. Is there anything I can do to help on this?

@mp911de
Copy link
Collaborator

mp911de commented Aug 31, 2020

Let's use the same estimation semantics for the UTF-8 case like netty does. That is calling ByteBufUtil.utf8MaxBytes(…) in sizeOf(…) when the codec uses utf8 mode.

@mp911de mp911de added this to the 6.0.0 milestone Sep 4, 2020
@mp911de mp911de merged commit 57489fb into redis:main Sep 4, 2020
mp911de added a commit that referenced this pull request Sep 4, 2020
Prefer exact estimations.

Original pull request: #1400.
mp911de added a commit that referenced this pull request Sep 4, 2020
Prefer exact estimations.

Original pull request: #1400.
@dmandalidis
Copy link
Contributor Author

@mp911de thanks, would it be possible to include it in an upcoming 5.3.x release? (no rush, we have a workaround)

@mp911de
Copy link
Collaborator

mp911de commented Sep 4, 2020

Sure, the fix is backported to 5.3 and we will release a new version in about a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants