Instantiators for additional container classes #4300
Conversation
…hSet, TreeSet, ConcurrentHashMap, and TreeMap. Replaced other new instantiator invocations with singletons.
src/main/java/com/fasterxml/jackson/databind/deser/impl/JDKValueInstantiators.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/impl/JDKValueInstantiators.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/impl/JDKValueInstantiators.java
Outdated
Show resolved
Hide resolved
| if (Collections.EMPTY_SET.getClass() == raw) { | ||
| return new ConstantValueInstantiator(Collections.EMPTY_SET); | ||
| return ConstantValueInstantiator.EMPTY_SET_INSTANCE; |
There was a problem hiding this comment.
No, I think these should not be pre-created since they are relatively rarely needed.
Let's leave them dynamically created on as-needed basis.
There was a problem hiding this comment.
Are they cached somewhere, or will they be instantiated for each deserialization call? I looked around, and it looked like the latter, which is why I made them all like that. If they are cached then it'd make sense to create them as we go and let cache handle the rest.
There was a problem hiding this comment.
They are actually cached as part of JsonDeserializer created, but not separately. End result should be the same tho.
I think eager creation is fine for default fallback types (ArrayList, LinkedHashMap, HashSet?).
Fine to also use for JsonLocation. These are more speculative.
There was a problem hiding this comment.
I gave it another spin. Default fallbacks remained eager, the rest moved to lazy. Seems like a sweet spot between eager and new.
There was a problem hiding this comment.
Ah. I should have been more careful here; by lazily I just meant creating instance directly with new -- effective caching is, as I said, indirect: actual JsonDeserializer created for specific type will use ValueInstantiator.
But I can actually make changes before merging so I think this is fine.
(also, due to the way class loaders work, I think final static instance fields are actually only constructed when class is initialized which does not occur -- AFAIK -- before "substantial" access (or whatever term was used), and not simply because class is referenced).
All I need now is the CLA, as per my note further down. Once that is done I can do whatever mas
src/main/java/com/fasterxml/jackson/databind/deser/impl/JDKValueInstantiators.java
Outdated
Show resolved
Hide resolved
|
@edudar Looks good overall, thank you for submitting this. One question tho: is the eager construction required to let GraalVM compiler find and included needed references? Second practical thing: before merging the pr we'd need CLA (unless you have sent one before). It's from here: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and easiest to print, fill & sign, scan/photo, email to |
cowtowncoder
added
the
cla-needed
|
@edudar Ok, I went ahead and made changes I think would make sense, wrt. only pre-creating/lazily-reusing instantiators for default mapped types. Please let me know if you have concerns. At this point I am ready to merge changes, but would need CLA that I mentioned above. It would also be good if you could test changed version against GraalVM image, just to make sure I didn't break anything there. Looking forward to merging this PR! |
cowtowncoder
removed
the
cla-needed
|
CLA received, hoping to merge soon. |
Took a stab at #4299
Added support for
LinkedList,HashSet,TreeSet,ConcurrentHashMap, andTreeMap.Also, replaced other new instantiator invocations with singletons.