Improve how we acquire ThreadContext to eliminate null refs.#5940
Improve how we acquire ThreadContext to eliminate null refs.#5940headius merged 2 commits intojruby:masterfrom
Conversation
|
previous version ended up recursing instead of always reaching a loop - otherwise seems the same? |
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
The actual scenario where you adopt and it returns null is pretty mysterious to me (very low memory?). If it does that once it feels like it could happen endlessly (and if so then perhaps we will get an issue showing just that). None the less this definitely gives it more of a chance to eventually adopt a TC.
|
@kares In the later comments of #5936 both @ahorek and I saw NPE when attempting to get the ThreadContext out of the SoftReference we acquired from the ThreadLocal. Somehow, that SoftReference was coming out as null even after calling The new logic basically just adds in extra null checking, in case the stars align to allow such to happen, and avoids recursing to re-attempt the acquisition of the context. It eliminates the NPE we saw. |
|
@enebo Yeah I couldn't trace a path that would result in getting a null SoftReference after adoption, but I could not exactly rule it out. There's two situations where that ThreadLocal might contain null:
In both cases the current thread should go into the logic that assigns a new SoftReference before proceeding. |
kares
left a comment
kares
left a comment
There was a problem hiding this comment.
minor improvement: do { ... } while since we always execute the loop at least once
that is also the way this method used to be before (the loop elimination).
|
@kares I will make that change, thank you. I want to understand if there was a reason you preferred recursion...something I'm missing in my version? My only concern with the recursion was that under some heavy GC scenario it may recurse many times, failing to acquire a context. It is not likely to happen or to cause stack issues, since we're talking soft references instead of weak references, but it nagged at the back of my brain a bit. The change was not altogether related to this patch, but it made sense to me. I don't like the overall complexity of the adoption logic, so I want to be absolutely sure I understand your version of this method. |
For jruby#5936 we saw some cases where the SoftReference created here came back as null, even after the "adopt" logic had run. While I have not isolated an exact cause, there are thread locals, soft references, and GC interacting here. Since the original logic did not guarantee non-null references throughout the code, I've modified it to always check and only proceed once it truly has non-null references and eventually context in hand.
headius
force-pushed
the
improved_threadcontext_acquisition
branch
from
8d37390 to
b529298
Compare
|
I've made the change and rebased to pick up fixes from master. |
think I did a micro-benchmark where it showed up that always entering a loop was 'slower'. anyway, its more important to have this working (even in edge cases) rather than 'optimized' to the ms which won't matter much in the end. |
|
@kares That seems plausible, but I think it shouldn't make much difference these days. It boils down to a branch versus a recursive invocation, and in both cases the rare path that fails to acquire a context should end up being profiled out. If you do see a difference, though, we can revisit it. |
3 participants
For #5936 we saw some cases where the SoftReference created here
came back as null, even after the "adopt" logic had run. While I
have not isolated an exact cause, there are thread locals, soft
references, and GC interacting here. Since the original logic did
not guarantee non-null references throughout the code, I've
modified it to always check and only proceed once it truly has
non-null references and eventually context in hand.