Improve memory usage via reuse InputStreamBufferInput #691

Activity

1. koo-taejin

   added a commit that references this issue on Nov 16, 2022

   [msgpack#691] Improve memory usage via reuse InputStreamBufferInput

   7e59e49
2. komamitsu

   self-assigned this

   on Jan 11, 2023
3. komamitsu commented on Jan 16, 2023

   

   komamitsu

   on Jan 16, 2023

   Member

   More actions

   @koo-taejin Great suggestion. Thanks for looking into the issue. And sorry for the late reply.
   I looked at koo-taejin@7e59e49 and the basic idea seems good. But I think there is room to improve it as follows:

   1. This MessageBufferInput (InputStreamBufferInput) cache can be treated as a part of existing reuseResourceInParser feature. So, we can avoid introducing the new feature flag reuseInputStreamBufferInput.
   2. Maintaining the different cache lifetimes of MessageUnpacker and MessageBufferInput would increase the complexity. We should keep using the same ThreadLocal to hold the new resource MessageBufferInput by change the value type from 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.
4. koo-taejin commented on Jan 17, 2023

   

   koo-taejin

   on Jan 17, 2023

   Author

   More actions

   @komamitsu
   I will try to change the code according to your suggestion. :)
   Thanks for letting me know for this project implementation more.
5. komamitsu commented on Jan 17, 2023

   

   komamitsu

   on Jan 17, 2023

   Member

   More actions

   Great to hear that!

   I think we need to delay to create a MessageBufferInput instance until

   msgpack-java/msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java

   Line 169 in edd0942

   if (!reuseResourceInParser) {

   . It means the private constructor needs to receive both of byte[] and InputStream (or either of them in a smart way) as argument.
6. koo-taejin

   added a commit that references this issue on Jan 19, 2023

   [msgpack#691] Improve memory usage via reuse InputStreamBufferInput

   56ac516
7. koo-taejin commented on Jan 19, 2023

   

   koo-taejin

   on Jan 19, 2023

   Author

   More actions

   @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 :)
8. komamitsu commented on Jan 19, 2023

   

   komamitsu

   on Jan 19, 2023

   Member

   More actions

   @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?

   👍React with 👍1Reacted by koo-taejin
9. koo-taejin commented on Jan 20, 2023

   

   koo-taejin

   on Jan 20, 2023

   Author

   More actions

   @komamitsu
   Absolutely yes, that is my honor. :)

   😄React with 😄1Reacted by Mitsunori Komatsu
10. komamitsu commented on Jan 21, 2023

    

    komamitsu

    on Jan 21, 2023

    Member

    More actions

    @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?
11. koo-taejin commented on Jan 25, 2023

    

    koo-taejin

    on Jan 25, 2023

    Author

    More actions

    @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 :)
12. komamitsu commented on Jan 25, 2023

    

    komamitsu

    on Jan 25, 2023

    Member

    More actions

    @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?