Add a way to configureSerializerCache Jackson uses
#4111
Conversation
JooHyukKim
changed the title
SerializerCache Jackson uses
| @@ -71,7 +80,8 @@ private final synchronized ReadOnlyClassToSerializerMap _makeReadOnlyLookupMap() | |||
| // not correctness | |||
| ReadOnlyClassToSerializerMap m = _readOnlyMap.get(); | |||
| if (m == null) { | |||
| m = ReadOnlyClassToSerializerMap.from(_sharedMap); | |||
| // TODO: Figure out how to pass something that can provides content-iterable without hard type casting. | |||
| m = ReadOnlyClassToSerializerMap.from((LRUMap<TypeKey, JsonSerializer<Object>>) _sharedMap); | |||
There was a problem hiding this comment.
This one needs more ideas. One option I can think of....
- Add new method that returns
Iterator<K,V>inLookupCache<K,V>. Possibly as a default method (for backward compatibility?) - Do some analysis and extract
content()method fromLRUMapor similiar type. Not sure on this one.
WDYT? /cc @cowtowncoder
EDITED : fixed bullet list format
There was a problem hiding this comment.
This is tricky, yes. I wonder what does master (3.0) do here? I think it has/had similar problem.
There was a problem hiding this comment.
Right, I missed checking master 🥲 lemme go check real quick
There was a problem hiding this comment.
There is no difference in master branch 🧐
There was a problem hiding this comment.
- Add new method that returns Iterator<K,V> in LookupCache<K,V>. Possibly as a default method (for backward compatibility?)
Would adding an absract method
Iterator<Map.Entry<K, V>> iterator();...be breaking other modules? Seems reasonable
There was a problem hiding this comment.
Well, master has this for LookupCache:
// 17-Sep-2019, tatu: There is one usage, by `ReadOnlyClassToSerializerMap`, so
// for now NOT exposed, but can reconsider if it proves generally useful
// void contents(BiConsumer<K,V> consumer);
so I think we could introduce such method, with default implementation of throwing UnsupportedOperationException.
The reason I prefer this is that it won't (have to) expose actual contents in a way that might allow modification.
src/main/java/com/fasterxml/jackson/databind/ser/DefaultSerializerProvider.java
Outdated
Show resolved
Hide resolved
|
Merged #4114 so that should help -- thank you for pointing out the missing piece! |
|
Retrofitted the change from #4114, and improved code level consistency with |
| } | ||
|
|
||
| @Test | ||
| public void testDefaultCacheProviderConfigSerializerCache() throws Exception |
There was a problem hiding this comment.
Not quite sure what is being tested here...
There was a problem hiding this comment.
Tests that with maxSerializerCacheSize set to positive value, works.
There was a problem hiding this comment.
Hmmmh. But it doesn't really check if it takes effect wrt changing cache size.
Just that nothing breaks in obvious way.
But I guess as sanity check, maybe extract common code that writes and reads output into helper method to reduce duplication.
| } | ||
|
|
||
| @Test | ||
| public void testDefaultCacheProviderConfigSerializerCacheSizeZero() throws Exception |
There was a problem hiding this comment.
Similarly, what exactly is being tested?
There was a problem hiding this comment.
Same here. Tests that with maxSerializerCacheSize set to zero, also works. Should we make test methods' names more meaningful or modify the tests themselves?
There was a problem hiding this comment.
Minor refactoring to reduce duplicated code would be useful in that case.
Although also checking that cache being constructed specified size would be useful -- but wouldn't want to add methods just for test cases.
So yeah, reducing code duplication would be good, I'm ok with the tests otherwise.
There was a problem hiding this comment.
Maybe later we can consider adding accessors somewhere. I guess we can go with deduplication for now 🥲
|
Getting close; I can hopefully merge this when I get back from my trip in 2 days. |
(Part of #2502)
Motification
Mostly similar to #4101 except
SeriailzerCacheconstructor where I added a note for.