-
Notifications
You must be signed in to change notification settings - Fork 321
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
Improve memory usage via reuse InputStreamBufferInput #691
Comments
@koo-taejin Great suggestion. Thanks for looking into the issue. And sorry for the late reply.
If you want to move forward on this improvement and create a PR, I hope the above idea sounds reasonable to you. If you're busy or not interested in creating a PR, I think I can take over this performance improvement as I have some time now. |
@komamitsu |
Great to hear that! I think we need to delay to create a msgpack-java/msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java Line 169 in edd0942
byte[] and InputStream (or either of them in a smart way) as argument.
|
Please check this PR is what you want. Thanks :) |
@koo-taejin Thanks. Looked over the branch (not PR, right?) Unfortunately, the change is too much more than I expected. Especially |
@komamitsu |
@koo-taejin I tried to reproduce the memory consumption difference with your change based on https://github.com/msgpack/msgpack-java/blob/develop/msgpack-jackson/src/test/java/org/msgpack/jackson/dataformat/benchmark/MessagePackDataformatPojoBenchmarkTest.java, but I didn't see any significant difference (I used |
I had tested to apply msgpack as a message protocol to spring mvc via following kotlin code.
thanks :) |
@koo-taejin Thanks for the information. But even based on the code you shared above, I don't think we can add tests code that is needed to see if the fix actually works. Do you think you can give me minimum code that reproduces how much your change improves the memory consumption? |
I am testing to use MsgPack instead of Jackson in Spring.
It was confirmed that MsgPack consumes more memory than Jackson.
I dug into that issue, it was confirmed that a lot of memory is created when creating
InputStreamBufferInput
.So I have make code very similar to the previous reuse form.
After the change, I had confirmed that the memory usage was greatly reduced, and the slope of the heap memory was also changed to be very stable.
I didn't add a test because I don't know whether you merge the code or not, but if you are going to accept it, I'll add a simple test code.
as-is
to-be
Thanks :)
The text was updated successfully, but these errors were encountered: