Open koo-taejin opened 1 year ago
@koo-taejin Great suggestion. Thanks for looking into the issue. And sorry for the late reply. I looked at https://github.com/koo-taejin/msgpack-java/commit/7e59e496fc1c233d06f0172ceaceefd100469017 and the basic idea seems good. But I think there is room to improve it as follows:
reuseResourceInParser
feature. So, we can avoid introducing the new feature flag reuseInputStreamBufferInput
.Tuple<>
to Triple<Object, MessageUnpacker, MessageBufferInput>
. I think this requires to modify some existing code.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 I will try to change the code according to your suggestion. :) Thanks for letting me know for this project implementation more.
Great to hear that!
I think we need to delay to create a MessageBufferInput
instance until https://github.com/msgpack/msgpack-java/blob/edd094282368d26fff7808ca792faa0ce488be61/msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java#L169. It means the private constructor needs to receive both of byte[]
and InputStream
(or either of them in a smart way) as argument.
@komamitsu
Please check this PR is what you want. If it is right, then I am going to add test code. And If you want to change my code, then you can change it viamaintainer option.
Thanks :)
@koo-taejin Thanks. Looked over the branch (not PR, right?) Unfortunately, the change is too much more than I expected. Especially core
component shouldn't be changed as much as possible since it's been carefully tuning considering JIT (e.g. Type Profile.) If it's okay, can I take over your branch and create a PR?
@komamitsu Absolutely yes, that is my honor. :)
@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 jcmd PID GC.class_histogram
.) Could you give me the test code to reproduce the memory consumption?
@komamitsu
I had tested to apply msgpack as a message protocol to spring mvc via following kotlin code.
val msgPackCoverter = object : AbstractJackson2HttpMessageConverter(
ObjectMapper(MessagePackFactory()),
MediaType("application", "x-msgpack")
) {}
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 :)