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

AMQP: Avoid allocating a new ByteString on each consumed message #592

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

dimamo5
Copy link

@dimamo5 dimamo5 commented Apr 2, 2024

Avoids copying a new array and a new allocation by using the fromArrayUnsafe method. It should be ok to build from this unsafe method as long as body is not exposed to the outside.

@pjfanning
Copy link
Contributor

Thanks @dimamo5 but I am not sure that this is necessarily a good idea. How can we be sure that AMQP provider does not reuse byte arrays? We can only use fromArrayUnsafe if we are 100% sure that the byte array won't be reused. Even if 1 AMQP solution can be proved not to reuse arrays, can we be sure that all AMQP solutions don't?

@dimamo5
Copy link
Author

dimamo5 commented Apr 2, 2024

The critical bit is the implementation of com.rabbitmq.client.Channel used in the DefaultConsumer. The default ChannelN and RecoveryAwareChannelN from rabbitmq client do not reuse byte arrays, since they eventually call https://github.com/rabbitmq/rabbitmq-java-client/blob/ca510a078092a3c64b1219feac527bc738b63313/src/main/java/com/rabbitmq/client/impl/CommandAssembler.java#L145 that performs a System.arraycopy.

Regarding the AMQP provider, I don't think this connector allows for customization of the channel directly from the provider so either one of the abovementioned channels will be used.

@pjfanning
Copy link
Contributor

RabbitMQ is not the only AMQP implementation. We could make the ByteString behaviour configurable but I don't think we can hardcore this change.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

Blocking merge while this change is debated.

dimamo5 and others added 2 commits April 20, 2024 01:07
Changes the ByteString constructor to a `fromArrayUnsafe`. It should be safe to use this unsafe method as long as `body` is to exposed to the outside.
@pjfanning
Copy link
Contributor

@dimamo5 I added an avoidArrayCopy setting that defaults to false (existing behaviour). I pushed this change to your PR, if you don't mind. Could you have a look?

If this approach is ok, new tests will need to be added - that test with avoidArrayCopy=true.

@dimamo5
Copy link
Author

dimamo5 commented May 15, 2024

@pjfanning sorry for the slow response. This is what I was looking for. Thank you.

@pjfanning
Copy link
Contributor

@dimamo5 I've added code so that message writing can also avoid copying the array data for the ByteStrings - also based on settings.avoidArrayCopy

I'm pretty busy with other Pekko stuff. Would you have time to add some test cases that say duplicate existing test cases for read and write cases but where settings.avoidArrayCopy is set to true?

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