GH-4084: Fix ByteBufferPool sizing in Fuseki's Jetty Server - #4088
GH-4084: Fix ByteBufferPool sizing in Fuseki's Jetty Server#4088divyankshah wants to merge 2 commits into
Conversation
Fuseki builds its Jetty Server with new Server(threadPool), which wires in a default ArrayByteBufferPool capped at 64KB. Fuseki configures a 5MB outputBufferSize, so output buffers exceed the pool's max capacity and are discarded on release instead of reused, causing continuous DirectMemory churn under load. Fixed in JettyServer.jettyServer(...), the shared factory both JettyHttps.java and FusekiServer.java call, so neither file needs changes. Constructs an ArrayByteBufferPool sized to FusekiSystemConstants.jettyOutputBufferSize and passes it into Server(ThreadPool, Scheduler, ByteBufferPool) at construction time, since adding the pool via addBean() afterward has no effect (the default pool is already wired in during construction).
| } | ||
| } | ||
|
|
||
| public static Server jettyServer(int minThreads, int maxThreads) { |
There was a problem hiding this comment.
Did we also want to address the other route that Andy mentioned on #4084 (comment) ?
There was a problem hiding this comment.
Hi @AniRamadoss, Thank you for asking - yes, it's covered. FusekiServer.java#L1907 calls
JettyServer.jettyServer(minThreads, maxThreads), the same shared factory I fixed. Since
the pool construction now lives inside that factory, both JettyHttps.java and
FusekiServer.java inherit the fix automatically also neither file needed direct changes,
it is dependent on JettyServer.java.
I confirmed this by tracing both call sites before submitting, also happy to paste the
exact call chain here if it's useful for review.
| * {@link FusekiSystemConstants#jettyOutputBufferSize}. */ | ||
| private static ByteBufferPool newByteBufferPool() { | ||
| int maxCapacity = FusekiSystemConstants.jettyOutputBufferSize; | ||
| return new ArrayByteBufferPool(0, 2048, maxCapacity, -1, -1, -1); |
There was a problem hiding this comment.
Any particular reason for the choice of 2048 here? (the default 4096)
There was a problem hiding this comment.
Hi @afs - Thank you for asking - I carried 2048 over from the suggested fix snippet in the original issue
report, without re-examining it against Fuseki's actual configured sizes. I just checked now:
all of Fuseki's buffer sizes (16KB header, 512KB request header, 5MB output) are exact
multiples of both 2048 and 4096.
I can switch it to -1 to defer to Jetty's own default (4096) rather than hardcoding a
value. Let me know if i should push that change?
There was a problem hiding this comment.
The fix snippet was for an older Jetty version.
We haven't considered whether we should decrease the output buffer size (not to 64k obviously, but maybe 2MB is enough). All the result set and RDF writers buffer as well. In the new async Jetty12 architecture, the ByteBufferPool seems to play the role of transferring byte from application to the output thread.
Without information about whether this issue is having a visible impact, it is a bit guesswork. A SPARQL operation itself has significantly more overhead than, say, serving a static web page - the effect of a 64K buffer may there but insignificant in all the noise.
There was a problem hiding this comment.
Hi @AniRamadoss - can we reproduce this and have logs for it for the current version?
There was a problem hiding this comment.
Hi @divyankshah, I'll take a look and will have an update for you by the end of the week.
There was a problem hiding this comment.
For output usage, the byte buffers are direct (out of heap - which in modern Java is not limited and can expand. Just don't call it "malloc" :-)).
For now, I think the setting should be :
4096 or jetty default -1. It probabley aligns with VM page size.
FusekiSystemConstants.jettyOutputBufferSize = 2M
5MB was just a large number. Large also has longer latency to first output.
Because the result set and RDF writers are already buffered for short strings and single-character writing, the Jetty buffers are cross thread transfers. Using several, not one big one, should be just as good.
(And yes - we could change the writers to allow buffer allocation to injected but one thing at a time and see what the buffering costs are anyway.)
There was a problem hiding this comment.
Thanks @afs, this makes sense - I'll make these changes and push shortly.
Co-authored-by: Andy Seaborne <andy@apache.org>
3 participants
Fixes #4084
Fuseki builds its Jetty
Serverwithnew Server(threadPool), which wires in adefault
ArrayByteBufferPoolcapped at 64KB. Fuseki configures a 5MBoutputBufferSize, so every output buffer exceeds the pool's max capacity and getsdiscarded on release instead of reused, causing continuous DirectMemory churn under
load. Full analysis in #4084.
This fixes it in
JettyServer.jettyServer(...), the shared factory bothJettyHttps.javaandFusekiServer.javacall, so neither file needs changes.Constructs an
ArrayByteBufferPoolsized toFusekiSystemConstants.jettyOutputBufferSizeand passes it intoServer(ThreadPool, Scheduler, ByteBufferPool)at construction time (adding the poolvia
addBean()afterward doesn't work, since the default pool is already wired induring construction).
Added a test asserting the built server's pool max capacity covers the configured
output buffer size. Full
jena-fuseki-mainsuite passes (799/799).