Cache types in ConstructorMapper #2650
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hgschmie
reviewed
Feb 22, 2024
| @@ -75,6 +75,9 @@ public final class ConstructorMapper<T> implements RowMapper<T> { | |||
| + "that your result set has the columns expected, annotate the " | |||
| + "parameter names explicitly with @ColumnName, or annotate nullable parameters as @Nullable"; | |||
|
|
|||
|
|
|||
| static final Map<Constructor, Type[]> CONSTRUCTOR_CACHE = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
would it be simpler to have that in the factory and make it private?
There was a problem hiding this comment.
I'll try that out, thanks!
There was a problem hiding this comment.
updated. Originally I was going to use a guava cache but I see that guava is only imported in test scope. lmk if you think it's a good idea to add a size limit, cache stats, ability to clear cache, disable cache (maybe for benchmarking), etc.
There was a problem hiding this comment.
We do not depend on Guava from Jdbi core, since it is a huge dep to push onto our users. Additionally, there are numerous known cache bugs that seemingly will never get fixed. The Guava CacheBuilder javadocs tell you to use Caffeine instead to avoid such bugs.
I think for a simple use case like this, a synchronized weak hash map is good enough. A map that caches JVM entities like constructors and classes will not grow without bound, so there is no need to size it.
elonazoulay
force-pushed
the
cache_types
branch
from
bb0bd84
to
2da68e8
Compare
|
Looks good. Lmk when you move it from draft to ready. |
|
Updated, lmk if it's good to go whenever you have a chance, thanks! |
| @@ -27,19 +29,18 @@ | |||
| import static java.util.Objects.requireNonNull; | |||
|
|
|||
| class ConstructorInstanceFactory<T> extends InstanceFactory<T> { | |||
| private final Constructor<T> constructor; | |||
| private static final Map<Constructor, Type[]> CONSTRUCTOR_CACHE = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
This will prevent the JVM from unloading any class that the ConstructorMapper ever considers, since it keeps a static strong reference to the Constructor.
Would the change work just as well with a synchronizedMap(new WeakHashMap<>()) or something like that? Then, the JVM would be able to unload classes since we do not strongly retain their Constructors.
There was a problem hiding this comment.
sounds good, will try all of these ideas out and update, thanks!
|
This is a broader change than the scope that you're trying to accomplish here, but I think the performance would be even better if we construct and cache a |
|
One last thought: it would be nice if we could see the improvement in a benchmark in |
elonazoulay
force-pushed
the
cache_types
branch
from
2da68e8
to
4024520
Compare
|
Updated, will run the benchmarks on the commit before the record constructor workaround, before the this commit and then after this commit and post the results to the pr. Thanks for all the ideas! |
elonazoulay
force-pushed
the
cache_types
branch
from
4024520
to
afafbbc
Compare
|
Just working through the ci failures, will update. |
|
Update: added a commit to cache the types and method handle in the instance factory. I'm running the |
elonazoulay
commented
Mar 2, 2024
|
|
||
| class ConstructorInstanceFactory<T> extends InstanceFactory<T> { | ||
| private final Constructor<T> constructor; | ||
| private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup(); |
There was a problem hiding this comment.
lmk if this shouldn't be cached.
There was a problem hiding this comment.
Let me know if you want the weak hashmap added back vs caching in the constructor instance factory (saved the previous version, so I can quickly update).
There was a problem hiding this comment.
So I think there is a bit of a misunderstanding here. I didn't mean to cache the lookup factory instance itself, but the constructorHandle instance itself.
Something like SynchronizedMap<WeakHashMap<ConstructorHandleAndTypes, MethodHandle>>> (referring to the key type from the other PR)
Sorry I wasn't clear, and sorry for taking so long to respond! Been a busy couple weeks at work.
elonazoulay
commented
Mar 2, 2024
|
|
||
| class ConstructorInstanceFactory<T> extends InstanceFactory<T> { | ||
| private final Constructor<T> constructor; | ||
| private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup(); |
There was a problem hiding this comment.
Let me know if you want the weak hashmap added back vs caching in the constructor instance factory (saved the previous version, so I can quickly update).
| @@ -207,14 +207,14 @@ private Optional<RowMapper<T>> createSpecializedRowMapper(StatementContext ctx, | |||
| List<String> unmatchedColumns) { | |||
| final int count = factory.getParameterCount(); | |||
| final Parameter[] parameters = factory.getParameters(); | |||
| final Type[] types = factory.getTypes(); | |||
| final List<Type> types = factory.getTypes(); | |||
There was a problem hiding this comment.
Had to convert to list due to error about exposing an array. Thinking to change the api to store the array internally and change the signature to getType(index), wdyt?
| @@ -44,10 +46,10 @@ Parameter[] getParameters() { | |||
| return executable.getParameters(); | |||
| } | |||
|
|
|||
| Type[] getTypes() { | |||
| List<Type> getTypes() { | |||
There was a problem hiding this comment.
same here - should it be Type getType(int index)?
|
|
Here are the benchmarks: Benchmark with the code in this pr: Benchmark for the weak hashmap version of this pr: Benchmark before the record constructor pr: |
|
lmk if you prefer this version or the weak hashmap version (static cache w synchronized weak hashmap). This version caches the method handle and types directly in the instance factory. |
Merged
|
Merging #2657 . Thanks! |
stevenschlansker
added a commit
that referenced
this pull request
Mar 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cache types to avoid expensive calculations in ConstructorInstanceFactory.
Following up on suggestions from #2648.
The constructor lookup should be quick:
hashCodeis class name andequalscompares parameters using object equality.