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

IPROTO-265 Avoid byte[] allocation for buffer of OutputStream #333

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

wburns
Copy link
Member

@wburns wburns commented Oct 3, 2024

@wburns
Copy link
Member Author

wburns commented Oct 3, 2024

Note this is a subset of the original commit at 16ba83f so that we avoid the byte[] that is used to buffer writes between the TagWriter and OutputStream as many implementations may not need this. In fact it is much simpler to instead wrap the OutputStream with their own BufferOutputStream if they want.

@wburns wburns force-pushed the avoid_buffer_for_outputstream branch from 9e4d888 to 1451186 Compare October 3, 2024 21:54
@pruivo pruivo added this to the 5.0.10.Final milestone Oct 4, 2024
@@ -154,7 +154,7 @@ public static ByteBuffer toWrappedByteBuffer(ImmutableSerializationContext ctx,
}

public static void toWrappedStream(ImmutableSerializationContext ctx, OutputStream out, Object t) throws IOException {
toWrappedStream(ctx, out, t, DEFAULT_STREAM_BUFFER_SIZE);
WrappedMessage.write(ctx, TagWriterImpl.newInstance(ctx, out), t);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be TagWriterImpl.newInstanceNoBuffer?

Copy link
Member

Choose a reason for hiding this comment

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

Ah! I see you changed newInstance(ImmutableSerializationContext serCtx, OutputStream output).
So newInstanceNoBuffer is kind of duplicated/irrelevant at this point!?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not aware of newInstanceNoBuffer! Yeah, no reason newInstance should ever use the buffered version imo. Updated to deprecate newInstanceNoBuffer as well.

@wburns wburns force-pushed the avoid_buffer_for_outputstream branch from 1451186 to 9349a40 Compare October 4, 2024 15:11
@pruivo pruivo merged commit 66a09ae into infinispan:main Oct 4, 2024
5 checks passed
@pruivo
Copy link
Member

pruivo commented Oct 4, 2024

merged! thanks @wburns !

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