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

Replace Jackson library by simple data I/O streams #8693

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 5, 2024

Pull Request Description

Improving #8553 by using DataOutputStream and DataInputStream instead of complex JSON serialization and deserialization.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Java,
    style guides.
  • All code has been tested:
    • Unit tests continue to pass
    • Remove Jackson from suggestion DB cache - done
    • Remove Jackson from import/export cache - done.
    • Benchmarks look better: first fix
    • Benchmarks look even better with three fixes

@JaroslavTulach JaroslavTulach self-assigned this Jan 5, 2024
@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 5, 2024
@Akirathan
Copy link
Member

Do you plan to also get rid of the JSON Object mapper in

private static final ObjectMapper objectMapper = new ObjectMapper();
and in
private static final ObjectMapper objectMapper = new ObjectMapper();
?

These are the only usages of com.fasterxml.jackson in runtime. We could then get rid of that dependency altogether.

@hubertp
Copy link
Contributor

hubertp commented Jan 5, 2024

Do you plan to also get rid of the JSON Object mapper in
...
These are the only usages of com.fasterxml.jackson in runtime. We could then get rid of that dependency altogether.

That could be nice, since their initialization is pretty expensive, even if done only once.

@JaroslavTulach
Copy link
Member Author

Looks like elimination of Jackson is a way to go. The startup decreased from 2940ms to 2780ms - e.g. 5% speedup:

empty startup

@JaroslavTulach JaroslavTulach changed the title Use simple data I/O streams instead of JSON 5% startup speed up by using simple data I/O streams instead of Jackson Jan 6, 2024
@JaroslavTulach
Copy link
Member Author

Do you plan to also get rid of the JSON Object mapper in

private static final ObjectMapper objectMapper = new ObjectMapper();

and in

private static final ObjectMapper objectMapper = new ObjectMapper();

?

Yes, let's do it! I've just added two tasks into this PR and I will do that by Monday.

These are the only usages of com.fasterxml.jackson in runtime. We could then get rid of that dependency altogether.

That would be great achievement! The less dependencies, the better.

Do you plan to also get rid of the JSON Object mapper in
...
These are the only usages of com.fasterxml.jackson in runtime. We could then get rid of that dependency altogether.

That could be nice, since their initialization is pretty expensive, even if done only once.

The only alternative is to switch to annotation processor based analysis like micronaut serde and eliminate the expensive initialization during runtime.

However, given how easy it is to use DataOutputStream for our purposes, we should really get of the JSON altogether.

@JaroslavTulach JaroslavTulach changed the title 5% startup speed up by using simple data I/O streams instead of Jackson Replace Jackson library by using simple data I/O streams Jan 8, 2024
@JaroslavTulach
Copy link
Member Author

The final results of last week speedups are visible in graph generated by:

enso/tools/performance/engine-benchmarks$ ./bench_download.py -s stdlib -b develop wip/jtulach/AvoidJacksonOnstartup_8553

Empty startup speedups

On Jan 2 the Startup_empty_startup benchmark was running in 3218ms. On Jan 8 it runs in 2794ms. Result of:

E.g. 13% speedup, congrats @Akirathan, @hubertp!

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/AvoidJacksonOnstartup_8553 branch from c777315 to f69efba Compare January 8, 2024 05:44
@JaroslavTulach JaroslavTulach changed the title Replace Jackson library by using simple data I/O streams Replace Jackson library by simple data I/O streams Jan 8, 2024
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 8, 2024
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Looks very nice. I also like the fact that the *.meta files in IR caches are still readable from a text editor.

@JaroslavTulach JaroslavTulach merged commit d86c6c4 into develop Jan 8, 2024
33 of 35 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/AvoidJacksonOnstartup_8553 branch January 8, 2024 12:17
@JaroslavTulach
Copy link
Member Author

To eliminate compile time dependency on Jackson isn't really easy. There is polyglot-api project which depends on Jackson. I've just created a branch wip/jtulach/NoCompileTimeJackson that shows a possible approach to eliminate such dependency:

  • splits polyglot-api project into two and creates engine-common
  • moves suggestions handling into runtime-suggestions project

The remaining problem is SerializationManager it needs access to suggestions, but it is also required to be in the runtime. Not sure how to solve this problem yet. As such I just report the status here, without attempting to move it forward right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants