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

Delay allocating Strings for Redis keyspace notifications #1708

Merged

Conversation

theigl
Copy link
Contributor

@theigl theigl commented Sep 26, 2020

Problem

Spring Session Redis allocates new Strings for channel and body of all incoming messages. In my case, Redis is used to store other session related data and RedisIndexedSessionRepository receives a lot of key expiration and deletion messages that it doesn't need to handle. Hundreds of megabytes of Strings are allocated unnecessarily every minute.

In such a case, it makes sense to delay allocating new Strings until we are sure the message should be handled by Spring session.

Solution

This PR directly uses the message's byte[] to check if a message should be handled. String allocation is delayed until we know that the message is intended for Spring Session.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 26, 2020
@theigl theigl force-pushed the reduce-message-allocations branch 2 times, most recently from 084d141 to ac6412f Compare September 26, 2020 15:14
@theigl
Copy link
Contributor Author

theigl commented Sep 26, 2020

I also created a related PR in Spring Data Redis that can further reduce the allocation rate for messages:

spring-projects/spring-data-redis#559

@theigl theigl force-pushed the reduce-message-allocations branch 3 times, most recently from 1c7ede0 to e914469 Compare September 26, 2020 20:04
@eleftherias eleftherias added in: redis type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 29, 2020
Copy link
Contributor

@vpavic vpavic left a comment

Choose a reason for hiding this comment

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

This looks like a nice optimization to me @theigl. 👍

@theigl
Copy link
Contributor Author

theigl commented Oct 1, 2020

@vpavic: I have a version of this in production already and it saves a couple of hundred MB of string allocations a minute. Our Redis is not really busy so this might make even more of a difference for other users.

Unfortunately, this is only half of the story because Message.getChannel() and Message.getBody() create new byte arrays. So filtering incoming messages is still not allocation free even after merging this PR. But it is a step in the right direction.

I'll ping the Spring Data guys to see if there is a chance my other PR will get merged. With both PRs applied, message filtering wouldn't allocate at all.

@theigl
Copy link
Contributor Author

theigl commented Oct 13, 2020

@vpavic: I discussed the situation with @mp911de and there is not much that can be done to improve the situation on the Spring Data side. So please go ahead and merge this PR if it is OK with you.

@vpavic vpavic self-assigned this Oct 31, 2020
@eleftherias eleftherias added this to the 2.5.0-M1 milestone Nov 10, 2020
@eleftherias eleftherias merged commit 5f51688 into spring-projects:master Nov 10, 2020
@theigl theigl deleted the reduce-message-allocations branch November 10, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: redis type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants