Skip to content

Improve memory usage via reuse InputStreamBufferInput #691

Open
Open
Improve memory usage via reuse InputStreamBufferInput#691
Assignees
komamitsu

Description

@koo-taejin
koo-taejin
opened on Nov 16, 2022

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
    msgpack as-is

  • to-be
    msgpack to be

Thanks :)

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
    komamitsu
    on Jan 16, 2023
    Member

    @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
    koo-taejin
    on Jan 17, 2023
    Author

    @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
    komamitsu
    on Jan 17, 2023
    Member

    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
    koo-taejin
    on Jan 19, 2023
    Author

    @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
    komamitsu
    on Jan 19, 2023
    Member

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

  9. koo-taejin commented on Jan 20, 2023

    @koo-taejin
    koo-taejin
    on Jan 20, 2023
    Author

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

  10. komamitsu commented on Jan 21, 2023

    @komamitsu
    komamitsu
    on Jan 21, 2023
    Member

    @koo-taejin I tried to reproduce the memory consumption difference with your change based on http://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
    koo-taejin
    on Jan 25, 2023
    Author

    @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
    komamitsu
    on Jan 25, 2023
    Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions