-
Notifications
You must be signed in to change notification settings - Fork 18
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
perf: Cache sql construction #522
Conversation
* avoid string concatenation and allocations * since they are now created each time its used due to the data partitions by slice
@@ -81,4 +82,29 @@ object Sql { | |||
} | |||
} | |||
|
|||
final class Cache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also this mutable.Map.getOrElseUpdate
from here - maybe this is more convenient, but I am not sure about the performance implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use immutable here since there will be no updates after initial population
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good idea
Completed this with caching in all places (where it makes sense) |
FROM $stateTable WHERE persistence_id = ?""" | ||
} | ||
protected def selectStateSql(slice: Int, entityType: String): String = | ||
sqlCache.get(slice, s"selectStateSql-$entityType") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when reviewing this, the highest risk of mistake (by me) is using the wrong cache key, such as accidentally using the same in several places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but way less sure it is a good idea at all after reviewing it. Esp methods that create a string with slice numbers in them in each call to avoid creating another string.
yeah, some might be questionable, but some might actually do much more than it looks, like those that create the slice ranges |
Let's merge it! |
@johanandren can you sanity check this before I continue?