Add a way to configure TypeFactory Jackson uses
#4115
Conversation
| @@ -205,10 +249,12 @@ public void testBuilderValueValidation() throws Exception | |||
| DefaultCacheProvider.builder() | |||
| .maxDeserializerCacheSize(0) | |||
| .maxSerializerCacheSize(0) | |||
| .maxTypeFactoryCacheSize(0) | |||
There was a problem hiding this comment.
One thing I wonder: I think that most cache implementations impose a minimum size, so 0 (for example) won't do what user might want -- that is, disabling of caching.
Then again, it is now possibly to add custom cache instances too which can be "no-op" (do nothing) so maybe this doesn't matter.
I also haven't tested out if backing LRUMap honors 0 size or not: JDK Maps do have minimum size but since 2.14 LRUMap uses integrated version of a more optimized caching library (ConcurrentLinkedHashMap).
There was a problem hiding this comment.
LRUMap does honor max size of 0 (a write occurs, but is immediately evicted). LRUMap is based off ConcurrentLinkedHashMap, which is a predecessor to Caffeine
Caffeine, on the other hand, batches evictions, so the zero maximum size constraint can be temporarily violated.
In addition, Cache2k, Android LruCache, and Ehcache all throw IllegalArgumentException when trying to create a cache with a maximum size of zero.
There was a problem hiding this comment.
Thank you @iProdigy . Sounds like we are good then. So 0 size for 2.14+ Jackson implementation will not retain anything in cache, and while there is write overhead that should be negligible.
It'd be nice to test this invariant, but no further work should be necessary it sounds.
There was a problem hiding this comment.
Hmmm, I am trying to come up with JavaDoc like...
* Note that setting value of {@code ...} to zero will not .... unlike some Cache providers.....but idk what to exactly say 🥲 any help?
There was a problem hiding this comment.
Added JavaDoc and tagged you there.
| * and {@link #_buildCache(int)} | ||
| * <p> | ||
| * Note that value zero is not considered a magic number, unlike some other libraries where it means "disabling cache". | ||
| * Instead setting value to zero only means setting cache size to zero. |
There was a problem hiding this comment.
Added JavaDoc as per comment #4115 (comment). WDYT?
There was a problem hiding this comment.
I would rephrase... For DefaultCacheProvider, zero max size effectively does disable caching since entries are not retained in the underlying LRUMap. Yes, it's technically true that a write & removal occurs under the hood, but users don't care about this detail (other than for optimizing performance, as a special no-op implementation could've been used instead)
Perhaps: Specifying a maximum size of zero prevents values from being retained in the cache
There was a problem hiding this comment.
but users don't care about this detail
True, they don't need to. Thank you for the suggestion! 🙏🏼🙏🏼 Changed accordingly
| * | ||
| * @since 2.16 | ||
| */ | ||
| public TypeFactory withCaches(CacheProvider cacheProvider) { |
There was a problem hiding this comment.
I think I'll change this so we won't need a new method -- caller can just construct LookupCache needed and pass that.
There was a problem hiding this comment.
Sure! Ill check your commit later👍🏼👍🏼
(part of #2502)
Blocked by #4111, which has implementations in cleaner form that can be retrofitted to this PR.