Skip to content
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

WIP iterative UntypedObjectDeserializer #3427

Closed
wants to merge 1 commit into from

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Mar 24, 2022

This is a WIP iterative implementation of UntypedObjectDeserializer. (see #3416 (comment) )

I looked at the existing JsonNodeDeserializer impl, and it is a bit better than this one (it uses a queue, and no intermediate worker objects, unlike this impl). However I don't think that approach can be easily adapted to work with arrays, it relies on adding the child collections to the parent immediately, which can be done with ArrayNode and could be done with List, but can't be done with arrays because they have fixed size (need to add to the parent after it's done).

Things left to do:

  • compatibility: probably need to keep the protected methods, and the Vanilla serializer class
  • perf: old impl had lots of small optimizations (e.g. for <=2-element lists), need to test how much perf this impl lost and do some optimizations for it
  • specialization: just one impl for now. old impl had a simplified "vanilla" impl with fewer checks (e.g. no special list deser). could do the same with this approach. could also use a separate queue-based impl (like JsonNodeDeserializer) for the case that doesnt need USE_JAVA_ARRAY_FOR_JSON_ARRAY

I've run the databind tests for this draft, and they pass, or at least don't fail more than without the changes. There are some tests that I think fail for me because I use java 17, not sure what's up with that, but they seem to be unaffected by these changes.

@@ -196,7 +195,7 @@ public void resolve(DeserializationContext ctxt) throws JsonMappingException
if ((_stringDeserializer == null) && (_numberDeserializer == null)
&& (_mapDeserializer == null) && (_listDeserializer == null)
&& getClass() == UntypedObjectDeserializer.class) {
return Vanilla.instance(preventMerge);
//return Vanilla.instance(preventMerge);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, Vanilla instance is performance-wise significantly faster than having to consider possible custom deserializers... so ultimately I'd like to retain this distinction. And/or at least measure performance difference in case I am wrong that there is significant difference.

@cowtowncoder
Copy link
Member

First of all: thank you for submitting this.

I find it bit tricky to review, due to diffs being... well it's non-diffable, due to nature of changes.
So once all this silly CVE nonsense dies down, I need to really understand code, changes.

I think my immediate concern is performance. I suspect changes would have non-trivial negative effect in throughput: existing implementation is quite optimized, guided by extensive benchmarking (using https://github.com/FasterXML/jackson-benchmarks/).
It might be good to benchmark throughput using jackson-benchmarks project's "Untyped" case, with PR and base 2.14 (or perhaps just 2.13 as 2.13 and 2.14 before changes should perform about the same).

@cowtowncoder
Copy link
Member

I ended up fixing this via #3473 which is sort of based on earlier JsonNode deserializer changes; closing this one.
Thank you for suggesting it; it helped with the actual changes!

@yawkat yawkat closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants