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

Deserialization improvements #620

Merged
merged 8 commits into from Oct 27, 2023
Merged

Deserialization improvements #620

merged 8 commits into from Oct 27, 2023

Conversation

dstepanov
Copy link
Contributor

Refactoring/rewriting of the deserialization:

  • Detection of the subtype is extracted to a separate deserializer with a newly introduced lookahead deserializer
  • SpecificObjectDeserializer rewritten to avoid buffering and to simplify the code

@dstepanov dstepanov marked this pull request as draft October 26, 2023 16:05
@dstepanov dstepanov marked this pull request as ready for review October 27, 2023 10:35
@yawkat
Copy link
Member

yawkat commented Oct 27, 2023

i dont like the LookaheadDecoder abstraction. I will try to find an alternative.

@dstepanov
Copy link
Contributor Author

Ok

@yawkat
Copy link
Member

yawkat commented Oct 27, 2023

@dstepanov take a look. This approach avoids new API on Decoder, and it uses the existing decodeBuffer so in theory the jackson implementation can be based on TokenBuffer for performance at some point.

Also do you think the changes to JsonNodeDecoder still have value now that the new subtypes are gone? If not, we can revert them for easier review

@dstepanov
Copy link
Contributor Author

dstepanov commented Oct 27, 2023

I did a bit of benchmarking of the unwrapped scenario: 10% faster than the previous code.

Vs Jackson:

Benchmark                                                (stack)  Mode  Cnt    Score    Error  Units
JacksonBenchmark.decodeUnwrapped  JACKSON_DATABIND_INTROSPECTION  avgt   10  660.406 ± 11.924  ns/op
JacksonBenchmark.decodeUnwrapped                   SERDE_JACKSON  avgt   10  554.094 ±  7.019  ns/op

@dstepanov
Copy link
Contributor Author

@yawkat Looks fine. I think there are some improvements added to JsonNodeDecoder, you can copy those subclasses back into the decoder if you want.

public @Nullable String decodeKey() throws IOException {
DemuxerState.Entry entry;
do {
entry = state.getEntry(nextKeyIndex++);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yawkat Can this be rewritten using an iterator?

Copy link
Member

Choose a reason for hiding this comment

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

no because the list may change size

@dstepanov
Copy link
Contributor Author

@yawkat IMHO it would be better if DemuxingObjectDecoder was split into two implementations, one that caches and one that replays / delegates. Now it's hard to understand with all the states there.

@yawkat
Copy link
Member

yawkat commented Oct 27, 2023

Do you mean just moving the nested classes or actually weakening the API?

@dstepanov
Copy link
Contributor Author

The best would be to have two implementation, the state can be passed as a map of string -> decoder

@yawkat
Copy link
Member

yawkat commented Oct 27, 2023

that'd be less powerful though, e.g. right now you can do more than two "passes", reading more properties each time. it would also not allow for concurrent reading though im not sure if that's useful.

but it'd also have a worse performance impact, right now if the first reader does finishStructure(true) the remaining properties are left undecoded and the second reader can access them without buffering. that cant be done if you wrap the full state in a Map.

@dstepanov
Copy link
Contributor Author

Ok let's keep it this way.

@sonarcloud
Copy link

sonarcloud bot commented Oct 27, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 15 Code Smells

75.9% 75.9% Coverage
1.1% 1.1% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@dstepanov dstepanov merged commit ef025fc into master Oct 27, 2023
11 of 12 checks passed
@dstepanov dstepanov deleted the ref branch October 27, 2023 18:46
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.

None yet

3 participants