[
https://issues.apache.org/jira/browse/PROTON-2994?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Julio J. Gomez Diaz updated PROTON-2994:
----------------------------------------
Description:
h2. Description
{{org.apache.qpid.proton.message.impl.MessageImpl}} keeps its
{{{}EncoderImpl{}}}/{{{}DecoderImpl{}}} in a {{{}private static final
ThreadLocal<EncoderDecoderPair> tlsCodec{}}}, created via
{{{}ThreadLocal.withInitial(...){}}}. This was introduced by PROTON-467 to
avoid the expensive {{registerAllTypes()}} on every
{{{}encode{}}}/{{{}decode{}}} call, and the reasoning still holds — but the
ThreadLocal shape has two drawbacks that matter on JDK 21+ with virtual threads:
# *It allocates per thread and is never reclaimed.* There is no {{remove()}}
anywhere in {{MessageImpl}} (or {{{}EncoderDecoderPair{}}}), so once a thread
calls {{{}encode{}}}/{{{}decode{}}} it holds the pair until the thread dies. On
a platform thread pool that is amortised; on virtual threads each new
carrier/runtime thread that encodes pays the full creation cost, and
short-lived threads keep the pair reachable until they terminate.
# *Measured cost.* On a fresh virtual thread a single {{Message.encode}}
allocates ~16.5 KB ({{{}gc.alloc.rate.norm{}}}, JMH), vs ~224 B on a warm
platform thread. Reproducer and numbers available (see below).
h2. Precedent
Apache Fory ships exactly this pattern and, for virtual threads, recommends the
pool over the ThreadLocal:
* {{Fory.builder().buildThreadLocalFory()}} → {{ThreadLocalFory}} (the shape
used here).
* {{Fory.builder().buildThreadSafeFory()}} → {{{}ThreadPoolFory{}}}, fixed
pool of {{{}4 * availableProcessors(){}}}.
* {{Fory.builder().buildThreadSafeForyPool(int)}} → explicit fixed pool size.
{{ThreadPoolFory}} picks a preferred slot by {{identityHashCode(thread) %
poolSize}} and falls back to any free slot ({{{}tryBorrowPreferredSlots{}}}),
releasing on completion — thread-agnostic, bounded memory. Fory's "Virtual
Threads" guide explicitly recommends the pool variant. Kryo has the equivalent
{{com.esotericsoftware.kryo.kryo5.util.Pool<T>}}
({{{}obtain(){}}}/{{{}free(){}}}).
Relevant prior discussion in this project:
* *PROTON-467* — introduced the ThreadLocal (Fixed).
* *PROTON-1410* — "Move MessageImpl::TLS to an utility class" (Won't Fix). In
that discussion tabish121 already floated the direction _"if the Encoders were
say made to not require being managed as thread locals for instance, e.g. we
made them stateless"_ — which is the same idea from the other end.
h2. Proposal (opt-in, default-preserving)
Keep {{tlsCodec}} as the default. Add an opt-in way to use a pooled/borrowable
pair:
* A static hook on {{{}MessageImpl{}}}, e.g.
{{MessageImpl.setCodecProvider(CodecProvider)}} (or a
{{{}setEncoderDecoderPool(...){}}}), where the provider supplies an
{{EncoderDecoderPair}} for the duration of an {{{}encode{}}}/{{{}decode{}}}
call and returns it afterwards. When unset, behaviour is unchanged
({{{}tlsCodec{}}}).
* A bounded pool implementation of that provider (fixed size, borrow/release),
so memory is capped by the pool size rather than by thread count.
This is additive; existing callers see no change.
Related: QPIDJMS-635 raises the same per-thread-codec concern for AmqpCodec's
own ThreadLocal (qpid-jms-client). The two are independent changes: each
library owns its own ThreadLocal and neither delegates to the other, so fixing
one does not fix the other. Linking them only so the shared rationale
(preferring a bounded pool over a per-thread instance for virtual-thread
workloads) is visible from both sides.
was:
h2. Description
{{org.apache.qpid.proton.message.impl.MessageImpl}} keeps its
{{EncoderImpl}}/{{DecoderImpl}} in a {{private static final
ThreadLocal<EncoderDecoderPair> tlsCodec}}, created via
{{ThreadLocal.withInitial(...)}}. This was introduced by PROTON-467 to avoid
the expensive {{registerAllTypes()}} on every {{encode}}/{{decode}} call, and
the reasoning still holds — but the ThreadLocal shape has two drawbacks that
matter on JDK 21+ with virtual threads:
# *It allocates per thread and is never reclaimed.* There is no {{remove()}}
anywhere in {{MessageImpl}} (or {{EncoderDecoderPair}}), so once a thread calls
{{encode}}/{{decode}} it holds the pair until the thread dies. On a platform
thread pool that is amortised; on virtual threads each new carrier/runtime
thread that encodes pays the full creation cost, and short-lived threads keep
the pair reachable until they terminate.
# *Measured cost.* On a fresh virtual thread a single {{Message.encode}}
allocates ~16.5 KB ({{gc.alloc.rate.norm}}, JMH), vs ~224 B on a warm platform
thread. Reproducer and numbers available (see below).
h2. Precedent
Apache Fory ships exactly this pattern and, for virtual threads, recommends the
pool over the ThreadLocal:
* {{Fory.builder().buildThreadLocalFory()}} → {{ThreadLocalFory}} (the shape
used here).
* {{Fory.builder().buildThreadSafeFory()}} → {{ThreadPoolFory}}, fixed pool of
{{4 * availableProcessors()}}.
* {{Fory.builder().buildThreadSafeForyPool(int)}} → explicit fixed pool size.
{{ThreadPoolFory}} picks a preferred slot by {{identityHashCode(thread) %
poolSize}} and falls back to any free slot ({{tryBorrowPreferredSlots}}),
releasing on completion — thread-agnostic, bounded memory. Fory's "Virtual
Threads" guide explicitly recommends the pool variant. Kryo has the equivalent
{{com.esotericsoftware.kryo.kryo5.util.Pool<T>}} ({{obtain()}}/{{free()}}).
Relevant prior discussion in this project:
* *PROTON-467* — introduced the ThreadLocal (Fixed).
* *PROTON-1410* — "Move MessageImpl::TLS to an utility class" (Won't Fix). In
that discussion tabish121 already floated the direction _"if the Encoders were
say made to not require being managed as thread locals for instance, e.g. we
made them stateless"_ — which is the same idea from the other end.
h2. Proposal (opt-in, default-preserving)
Keep {{tlsCodec}} as the default. Add an opt-in way to use a pooled/borrowable
pair:
* A static hook on {{MessageImpl}}, e.g.
{{MessageImpl.setCodecProvider(CodecProvider)}} (or a
{{setEncoderDecoderPool(...)}}), where the provider supplies an
{{EncoderDecoderPair}} for the duration of an {{encode}}/{{decode}} call and
returns it afterwards. When unset, behaviour is unchanged ({{tlsCodec}}).
* A bounded pool implementation of that provider (fixed size, borrow/release),
so memory is capped by the pool size rather than by thread count.
This is additive; existing callers see no change.
> Allow a pooled/reusable codec instead of the per-thread AmqpCodec ThreadLocal
> -----------------------------------------------------------------------------
>
> Key: PROTON-2994
> URL: https://issues.apache.org/jira/browse/PROTON-2994
> Project: Qpid Proton
> Issue Type: Improvement
> Components: proton-j
> Affects Versions: proton-j-0.35.0
> Reporter: Julio J. Gomez Diaz
> Priority: Major
>
> h2. Description
> {{org.apache.qpid.proton.message.impl.MessageImpl}} keeps its
> {{{}EncoderImpl{}}}/{{{}DecoderImpl{}}} in a {{{}private static final
> ThreadLocal<EncoderDecoderPair> tlsCodec{}}}, created via
> {{{}ThreadLocal.withInitial(...){}}}. This was introduced by PROTON-467 to
> avoid the expensive {{registerAllTypes()}} on every
> {{{}encode{}}}/{{{}decode{}}} call, and the reasoning still holds — but the
> ThreadLocal shape has two drawbacks that matter on JDK 21+ with virtual
> threads:
> # *It allocates per thread and is never reclaimed.* There is no {{remove()}}
> anywhere in {{MessageImpl}} (or {{{}EncoderDecoderPair{}}}), so once a thread
> calls {{{}encode{}}}/{{{}decode{}}} it holds the pair until the thread dies.
> On a platform thread pool that is amortised; on virtual threads each new
> carrier/runtime thread that encodes pays the full creation cost, and
> short-lived threads keep the pair reachable until they terminate.
> # *Measured cost.* On a fresh virtual thread a single {{Message.encode}}
> allocates ~16.5 KB ({{{}gc.alloc.rate.norm{}}}, JMH), vs ~224 B on a warm
> platform thread. Reproducer and numbers available (see below).
> h2. Precedent
> Apache Fory ships exactly this pattern and, for virtual threads, recommends
> the pool over the ThreadLocal:
> * {{Fory.builder().buildThreadLocalFory()}} → {{ThreadLocalFory}} (the shape
> used here).
> * {{Fory.builder().buildThreadSafeFory()}} → {{{}ThreadPoolFory{}}}, fixed
> pool of {{{}4 * availableProcessors(){}}}.
> * {{Fory.builder().buildThreadSafeForyPool(int)}} → explicit fixed pool size.
> {{ThreadPoolFory}} picks a preferred slot by {{identityHashCode(thread) %
> poolSize}} and falls back to any free slot ({{{}tryBorrowPreferredSlots{}}}),
> releasing on completion — thread-agnostic, bounded memory. Fory's "Virtual
> Threads" guide explicitly recommends the pool variant. Kryo has the
> equivalent {{com.esotericsoftware.kryo.kryo5.util.Pool<T>}}
> ({{{}obtain(){}}}/{{{}free(){}}}).
> Relevant prior discussion in this project:
> * *PROTON-467* — introduced the ThreadLocal (Fixed).
> * *PROTON-1410* — "Move MessageImpl::TLS to an utility class" (Won't Fix).
> In that discussion tabish121 already floated the direction _"if the Encoders
> were say made to not require being managed as thread locals for instance,
> e.g. we made them stateless"_ — which is the same idea from the other end.
> h2. Proposal (opt-in, default-preserving)
> Keep {{tlsCodec}} as the default. Add an opt-in way to use a
> pooled/borrowable pair:
> * A static hook on {{{}MessageImpl{}}}, e.g.
> {{MessageImpl.setCodecProvider(CodecProvider)}} (or a
> {{{}setEncoderDecoderPool(...){}}}), where the provider supplies an
> {{EncoderDecoderPair}} for the duration of an {{{}encode{}}}/{{{}decode{}}}
> call and returns it afterwards. When unset, behaviour is unchanged
> ({{{}tlsCodec{}}}).
> * A bounded pool implementation of that provider (fixed size,
> borrow/release), so memory is capped by the pool size rather than by thread
> count.
> This is additive; existing callers see no change.
>
> Related: QPIDJMS-635 raises the same per-thread-codec concern for AmqpCodec's
> own ThreadLocal (qpid-jms-client). The two are independent changes: each
> library owns its own ThreadLocal and neither delegates to the other, so
> fixing one does not fix the other. Linking them only so the shared rationale
> (preferring a bounded pool over a per-thread instance for virtual-thread
> workloads) is visible from both sides.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
