-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
CharacterReader always allocate a 32 KB buffer that can even exceed document size #1773
Comments
I think the difficulty will be in knowing upfront the expected content length of the response to parse. My understanding in practice is that if it is set, the Content Length header is often incorrect. And so browsers typically ignore it / handle larger sizes. Have you actually profiled this to be a performance issue? I picked that number based on a bit of empirical benchmarking and testing and found it worked reasonably. Another approach would be to reuse the buffer on subsequent reads, so the allocation doesn't need to happen. We use that pattern in e.g. StringBuilders. |
No not that critical, though you get many small HTLM emails in an email server. |
OK yes in that use-case I think it would make sense. A pattern of a threadlocal reusable buffer would work, similar to the Builders in StringUtil. |
Thinking more about this, is there something that prevent doing (potentially opt-in) buffer recyclingjust like Jackson JSON parsers does ? I am working on a similar approach in MIME4J and it did so far yield a 40& allocation reduction that translated in micro-benchmarks into a 10-15% performance improvment. I would be happy to cary other such a contribution. |
I'm not familiar with the implementation of Jackson parser. Recyling the buffer makes sense. I don't know that it should be opt-in as I would expect most people to miss it if it were. Would be happy to review a PR! |
Question before getting started: are there some jmh benchmarks somewhere? Would you be interested in getting a set of jmh benchmarks as well? Will be in vacations all of july so nothing would likely happen before august. |
I did succeed to put together #1800 before leaving. Looks like massive gains are ahead ;-) |
This might be overkill and we might be able to decrease the size of this buffer for small strings to be parsed.
This should thus reduce memory allocation.
Here is a small async profiler memory flame graph showing this:
The text was updated successfully, but these errors were encountered: