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

Improve memory usage via reuse InputStreamBufferInput #691

Open
koo-taejin opened this issue Nov 16, 2022 · 9 comments
Open

Improve memory usage via reuse InputStreamBufferInput #691

koo-taejin opened this issue Nov 16, 2022 · 9 comments
Assignees

Comments

@koo-taejin
Copy link

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 :)

koo-taejin added a commit to koo-taejin/msgpack-java that referenced this issue Nov 16, 2022
@komamitsu komamitsu self-assigned this Jan 11, 2023
@komamitsu
Copy link
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.

@koo-taejin
Copy link
Author

@komamitsu
I will try to change the code according to your suggestion. :)
Thanks for letting me know for this project implementation more.

@komamitsu
Copy link
Member

Great to hear that!

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

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

koo-taejin added a commit to koo-taejin/msgpack-java that referenced this issue Jan 19, 2023
@koo-taejin
Copy link
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 :)

@komamitsu
Copy link
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?

@koo-taejin
Copy link
Author

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

@komamitsu
Copy link
Member

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

@koo-taejin
Copy link
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 :)

@komamitsu
Copy link
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants