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 thread-safety of buffer recycling #479

Closed
cowtowncoder opened this issue Sep 18, 2018 · 7 comments
Closed

Improve thread-safety of buffer recycling #479

cowtowncoder opened this issue Sep 18, 2018 · 7 comments
Milestone

Comments

@cowtowncoder
Copy link
Member

(note: follow-up to #476)

ThreadLocal based buffer recycling worked well for traditional blocking i/o where usage of a given parser or generator is usually (although not always!) limited to just a single blocking thread.
And although in theory it would be possible to have systems that use sequence of threads for processing (for example first worked thread processing part of heard), we have observed no actual use cases.
However, with async/non-blocking parsing, it is more likely that something similar occurs, in which -- for example -- one thread might process events for a single buffer-full of content.

This is problematic not because multiple threads try to access explicitly shared resource -- they won't -- but because ownership model gets twisted due to ThreadLocal access by parsers, generators, so that effectively BufferRecycler that should be "owned" by just one parser / generator at any given point in time may end up getting actually shared. And at this point lack of synchronization (which was assumed not needed due to ThreadLocal forcing access from just one thread -- not true if parser/generator accessed from multiple!) will allow sharing of actual underlying buffers.

We can solve this in multiple ways, but probably the first attempt should be changing array-access of BufferRecycler fields to use atomic accessors. This adds some overhead, which is likely negligible (only lock when obtain, release), although will probably reduce efficiency of actual recycling more. Still, it should be more efficient than no recycling, and actually safe, as opposed to corruption due to lack of sync.

@re-thc
Copy link

re-thc commented Feb 21, 2019

Maybe better / easier to do something like a ringbuffer? i.e. what LMAX Disruptor does? ThreadLocal isn't nice when you have too many threads and it eats up memory anyhow.

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Feb 21, 2019

@hc-codersatlas For anything more complicated I'd just create a plug-in point and let whoever cares do it. Actual use cases and requirements for devs who do care are just complicated enough that I'm not sure it's practical to do that. So not saying it's a bad idea, just that I don't trust my skills to implement or even review high-quality compact solution.

So I don't think I want to add global/shared caching scheme, but rather let anyone who wants to do just that.
And leave default to 2 options (use thread-specific soft/weak-reference set of buffers; do not recycle anything).

@sdeleuze
Copy link

As part of the upcoming Spring Framework 5.2, we are very interested by a solution that would allow us to plug our NettyDataBufferFactory to use pooled Netty ByteBuf exposed as ByteBuffer.

Could you provide us such extension point?

@cowtowncoder
Copy link
Member Author

@sdeleuze I think worth filing a separate issue, but one question: Jackson does not use ByteBuffers internally for anything, so how would this plug in?

@cowtowncoder
Copy link
Member Author

So, for 2.10, replaced unsynchronized access with java.util.concurrent.atomic.AtomicReferenceArray -- a very simple change, and unlikely to have measurable negative performance effect(s) (and from casual run of jackson-benchmarks for reading, saw absolutely no effect).
With that, enabled recycling for async/non-blocking parser again.

Also got rid of all recycling for JsonStringEncoder (since it's only used for getting SerializedString). For 3.0 (and 2.11 if we do that) will probably try to add fully pluggable abstraction for fetching BufferRecyclers using whatever mechanism, could be pooling and not ThreadLocal. Or, if somehow I end up having extra time might even still squeeze in 2.10 but only if it gets in a pr for thorough vetting.

@sdeleuze
Copy link

Thanks @cowtowncoder for that, much appreciated.

@cowtowncoder
Copy link
Member Author

@sdeleuze I am happy to be able to reduce some of unnecessary complexity! 2.10 will also reduce some of maximum recyclable buffer sizes which can help reduce memory retention (per-thread) quite a bit.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Aug 28, 2019
Since now FasterXML/jackson-core#476
and FasterXML/jackson-core#479 are fixed.

This commit also raises the minimum version of Jackson to 2.9.7.

Closes spring-projectsgh-23522
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Aug 28, 2019
Since now FasterXML/jackson-core#476
and FasterXML/jackson-core#479 are fixed.

This commit also raises the minimum version of Jackson to 2.9.7.

Closes spring-projectsgh-23522
@cowtowncoder cowtowncoder changed the title Improve thread-safety of buffer recycling to enable recycling again for async parsing Improve thread-safety of buffer recycling Sep 19, 2019
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

3 participants