-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
i dont like the LookaheadDecoder abstraction. I will try to find an alternative. |
Ok |
@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 |
I did a bit of benchmarking of the unwrapped scenario: 10% faster than the previous code. Vs Jackson:
|
@yawkat Looks fine. I think there are some improvements added to |
public @Nullable String decodeKey() throws IOException { | ||
DemuxerState.Entry entry; | ||
do { | ||
entry = state.getEntry(nextKeyIndex++); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
serde-support/src/main/java/io/micronaut/serde/support/deserializers/DemuxingObjectDecoder.java
Outdated
Show resolved
Hide resolved
@yawkat IMHO it would be better if |
Do you mean just moving the nested classes or actually weakening the API? |
The best would be to have two implementation, the state can be passed as a map of string -> decoder |
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. |
Ok let's keep it this way. |
SonarCloud Quality Gate failed. 0 Bugs 75.9% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Refactoring/rewriting of the deserialization:
SpecificObjectDeserializer
rewritten to avoid buffering and to simplify the code