Improvements in ThreadLocalStorage

[dpsoft]

• Wrapper that implements optimized ThreadLocal access pattern ideal for heavily used ThreadLocals.

• It is faster to use a mutable holder object and always perform ThreadLocal.get() and never use ThreadLocal.set(), because the value is more likely to be found in the ThreadLocalMap direct hash
  slot and avoid the slow path ThreadLocalMap.getEntryAfterMiss().

• With a twist proposed by @adriancole where we don't use an Kamon-defined holder object as that would prevent class unloading.

JMH Results:

taskset -c 0-2 sbt 'coreBench/jmh:run -i 10 -wi 5 -t2 .*Benchmark.*'

[info] Benchmark                                       Mode  Cnt   Score   Error  Units
[info] ThreadLocalStorageBenchmark.currentThreadLocal  avgt  100  17.579 ± 0.261  ns/op
[info] ThreadLocalStorageBenchmark.fastThreadLocal     avgt  100  13.769 ± 0.275  ns/op

[dpsoft]

All credits to FastThreadLocal by @trask

[trask]

👍

[codefromthecrypt]

the classloader thing happens even if you clear every time.. it is a weird one! we have a kindof horrible framework to show this problem Ex this test will fail even if you clear the ref.. the test below will fail until it is a weak ref, but weak ref isn't cheap either.. https://github.com/openzipkin/brave/blob/master/brave-tests/src/main/java/brave/test/propagation/CurrentTraceContextTest.java#L265 https://github.com/openzipkin/brave/blob/master/brave-tests/src/main/java/brave/test/util/ClassLoaders.java

…

On Wed, Sep 26, 2018 at 7:27 PM Diego Parra ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In kamon-core/src/main/scala/kamon/context/Storage.scala <#553 (comment)>: > class ThreadLocal extends Storage { - private val tls = new java.lang.ThreadLocal[Context]() { - override def initialValue(): Context = Context.Empty + private val tls = new java.lang.ThreadLocal[ContextHolder]() { + override def initialValue() = new ContextHolder(Context.Empty) @adriancole <https://github.com/adriancole> thanks for pointing that, yes you are right, here we can have a classloader leak if the context is not correctly closed, what do you think about something like this: final class ContextHolder(var value:Context) extends WeakReference(value) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD611CZHJERduiTNsyq8EWcNLKOczBXks5ufA1GgaJpZM4WkcRy> .

[codefromthecrypt]

cheap being relative to a plain ref or array access I mean.. not saying weakref isn't cheap enough

…

On Wed, Sep 26, 2018 at 7:34 PM Adrian Cole ***@***.***> wrote: the classloader thing happens even if you clear every time.. it is a weird one! we have a kindof horrible framework to show this problem Ex this test will fail even if you clear the ref.. the test below will fail until it is a weak ref, but weak ref isn't cheap either.. https://github.com/openzipkin/brave/blob/master/brave-tests/src/main/java/brave/test/propagation/CurrentTraceContextTest.java#L265 https://github.com/openzipkin/brave/blob/master/brave-tests/src/main/java/brave/test/util/ClassLoaders.java On Wed, Sep 26, 2018 at 7:27 PM Diego Parra ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In kamon-core/src/main/scala/kamon/context/Storage.scala > <#553 (comment)>: > > > class ThreadLocal extends Storage { > - private val tls = new java.lang.ThreadLocal[Context]() { > - override def initialValue(): Context = Context.Empty > + private val tls = new java.lang.ThreadLocal[ContextHolder]() { > + override def initialValue() = new ContextHolder(Context.Empty) > > @adriancole <https://github.com/adriancole> thanks for pointing that, > yes you are right, here we can have a classloader leak if the context is > not correctly closed, what do you think about something like this: > > final class ContextHolder(var value:Context) extends WeakReference(value) > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#553 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAD611CZHJERduiTNsyq8EWcNLKOczBXks5ufA1GgaJpZM4WkcRy> > . >

[ivantopo]

Couple stupid comments, but in general looks good to me! Thanks @adriancole for the support and @trask for the original awesomeness :D