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 streams with loops in DefaultMessagePackMapper #439

Merged
merged 7 commits into from
Jan 19, 2024

Conversation

akudiyar
Copy link
Collaborator

@akudiyar akudiyar commented Nov 12, 2023

toValue and fromValue methods in DefaultMessagePackMapper are hot spots as they are called on each request. This code was first written using Java streams, but while it allowed to make the code more readable, the streams are not very well-suitable for this task. The Java streams work better when we can execute the chained lambda functions in parallel on an executor, but are worse when replacing the normal for loops - they add significant overhead because of a lot of created auxiliary classes and objects in runtime.

Now as the DefaultMessagePackMapper code is stable we can replace the stream-based code with more optimized loop-based code.

@akudiyar
Copy link
Collaborator Author

Waits for #438 to be merged.

@akudiyar akudiyar marked this pull request as ready for review December 28, 2023 10:13
Copy link
Contributor

@ArtDu ArtDu left a comment

Choose a reason for hiding this comment

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

LGTM, only one request: add lines in changelog for this PR and for the prev one #438

The stream-based code was more readable, but generated a lot of
auxiliary classes and objects in runtime, making a significant GC
overhead.
The stream-based code was more readable, but generated a lot of
auxiliary classes and objects in runtime, making a significant GC
overhead.
@akudiyar akudiyar force-pushed the replace-streams-with-loops-tmp branch 2 times, most recently from b89828d to 5015fc6 Compare January 5, 2024 00:47
Use ArrayList in converter maps to utilize sequential loops over
the internal array instead of creating an iterator instance every time
which adds additional GC pressure.

Also improve maps encoding by creating an immediate zero-copy structure.
References to temporary objects were used in some places for request
signatures calculation. Replace them with input parameters.
@akudiyar akudiyar force-pushed the replace-streams-with-loops-tmp branch from 5015fc6 to 5f9e69c Compare January 14, 2024 19:24
@akudiyar
Copy link
Collaborator Author

Changelog line added in #453

I guess we can merge this PR as it is.

@akudiyar akudiyar added this pull request to the merge queue Jan 19, 2024
Merged via the queue into master with commit 8a4bcde Jan 19, 2024
3 checks passed
@akudiyar akudiyar deleted the replace-streams-with-loops-tmp branch January 19, 2024 21:06
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