DATAREDIS-1212 Avoid allocation when accessing DefaultMessage channel and body

[theigl]

This PR avoids allocating new byte arrays when accessing DefaultMessage.getChannel and DefaultMessage.getBody.

DefaultMessage.channelStartsWith and DefaultMessage.bodyStartsWith are added so Spring Session can process messages without allocations:

https://github.com/spring-projects/spring-session/blob/0f3ea33b50678a764a50f7851b86fa45a99d2c85/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java#L503

See https://jira.spring.io/browse/DATAREDIS-1212

[pivotal-issuemaster]

@theigl Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@theigl Thank you for signing the Contributor License Agreement!

[theigl]

Note: The new methods hasChannel and hasBody also check if the underlying byte arrays are empty, so this slightly changes the behavior of these conditionals.

[theigl]

@mp911de: My related PR for Spring Session was just approved. Could you give me some feedback on the changes proposed in this PR? If this was merged, filtering incoming messages in Spring Session would be completely allocation free.

[mp911de]

Avoiding allocations is a fine and welcome enhancement. I think we can optimize for the empty case by having an empty array constant. getChannel and getBody are expected to be non-null which allows for getting rid of some null checks.

Introducing additional methods on the interface would start cluttering the interface and we want to avoid such a development.
Returning ByteArrayWrapper promotes ByteArrayWrapper to an API type which holds a direct reference to the array types and allows for modifications of the array.

That being said, let's change this pull request to optimize for empty byte arrays first and try removing the isEmpty checks in KeyspaceEventMessageListener to learn how much effect we can get from this change.

[theigl]

@mp911de: Thanks for your feedback!

Avoiding allocations is a fine and welcome enhancement. I think we can optimize for the empty case by having an empty array constant.

I'm not sure I can follow you. How can we compare against the empty array constant without calling getMessage() and getBody()? Could you please elaborate so I can make the changes?

Introducing additional methods on the interface would start cluttering the interface and we want to avoid such a development.
Returning ByteArrayWrapper promotes ByteArrayWrapper to an API type which holds a direct reference to the array types and allows for modifications of the array.

I agree. It didn't feel right to add these methods, especially the one referencing ByteArrayWrapper. Unfortunately, this is the only way I could think of to get rid of this allocation. I'll revert this change.

[mp911de]

I'm not sure I can follow you. How can we compare against the empty array constant without calling getMessage() and getBody()? Could you please elaborate so I can make the changes?

I think I wasn't really clear where I wanted this PR to go. message and channel can't ever be null, however, they can be empty. We have null checks that call getMessage() and getChannel(). By removing these we avoid cloning the byte arrays in DefaultMessage which eliminates garbage.

Introducing constants for empty byte arrays is a tiny change that we can introduce without exposing additional methods. My aim is to see how much we gain from eliminating null checks and the improvements in Spring Session to learn how much more we need to optimize. Each optimization is associated with cost and we're not keen on introducing additional methods to be zero-alloc for a specific use-case.

[theigl]

@mp911de: Thanks for clarifying this! We can definitely remove the null checks. This will reduce allocations for everyone using RedisKeyValueAdapter.

The problem is that Spring Session directly uses RedisMessageListenerContainer for key expiration messages. This class allocates the channel twice:

• spring-data-redis/src/main/java/org/springframework/data/redis/listener/RedisMessageListenerContainer.java

  Line 973 in 0f54fcf

  listeners = channelMapping.get(new ByteArrayWrapper(message.getChannel()));

• spring-data-redis/src/main/java/org/springframework/data/redis/listener/RedisMessageListenerContainer.java

  Line 983 in 0f54fcf

  final byte[] source = (pattern != null ? pattern.clone() : message.getChannel());

There really isn't much that can be done about these two allocations. Since I know that the message channel and body are never changed in my application, I'll override my Redis Subscription and return my own, non-allocating version of a Message. Together with my PR in Spring Session, this should get rid of all unnecessary allocations.

I'll adjust this PR to simply remove the null-checks in RedisKeyValueAdapter.

[mp911de]

Sounds good. Let us know whether there's a need to make extension points protected/public so you can easier hook into customizations.

[theigl]

@mp911de: I adjusted the PR to only remove the redundant null-checks in RedisKeyValueAdapter.

Sounds good. Let us know whether there's a need to make extension points protected/public so you can easier hook into customizations.

I'm using Redisson where this is relatively easy to achieve. Supplying your own Message implementation for Lettuce or Jedis is nearly impossible at the moment. LettuceMessageListener and JedisMessageListener, which create the DefaultMessage, are package private and their instantiation cannot be controlled.

It would be great to have some kind of pluggable MessageFactory. But since no one else has complained about these unnecessary allocations, it might not be worth to pursue this further. 😉

[mp911de]

Thank you for your contribution. That's merged, polished, and backported now.