-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
do not eagerly set eval-type in a thread-local #4215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear in a couple of years someone will revert this without realizing this was done for a reason. @kares could you write a comment about why we want it default to null vs NONE?
My only thought is since this is a protected field do we have any subclasses directly accessing this field?
b33a81f
to
0badff8
Compare
yes there's MixedModelIRBlockBody ... also the reason why the field is not final I guess. have added a comment about TC and amended the commit, thanks for the feedback. |
@kares ah yeah that guy. So that copies the field to another so if we for some reason changed that to getEvalType() in the future we would start seeing the Tomcat unload errors again (possibly). I have no suggestion for that. I guess over time someone else will notice we are using it wrong and we will add another comment somewhere. |
@enebo not sure but I think in that case the thread-local instance sharing is essential - changing to |
I'm not very familiar with JRuby - in fact, I have no idea what this class is used for under the covers - but I am seeing thousands upon thousands of these error messages in our tomcat logs with some JRuby apps created on a project I'm working on. In looking at the patch, I can see that this may improve things, but I'm wondering if it will actually remove the leak entirely? Is some sort of pattern set, where upon each call to And why isn't evalType static? As a java developer with no specific knowledge of how this class is used, a cursory code review makes me think that Tomcat is indeed correct, and https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/runtime/IRBlockBody.java is creating a leak. |
no it won't avoid the leak since it still might be set - but TC can clear the thread-local map for you I believe. having it static would make things "simpler" - on runtime |
Is this class a singleton in its usage pattern? Or is it created per request? |
In reviewing this I think we did this to prevent storing this on our artificial Frame object. N threads can execute the same block in different eval contexts. How the code is written we do not pass in this eval type context as part of the call/yield code path (which would have been a nice way to solve this). Maybe this is a possibility to solve for reals? Adding to our frame may or may not even be enough as we eliminate frames in cases but we may still need this value. We have N possible nested block invocations in a call stack so we cannot use a single value on threadcontext unless we pay the load/store cost (which @kares alluded to). That solution can also be error prone if you do not guard all possible places where you use/set that value. It would get rid of the leak issue since we would not use a threadlocal. It also is only a single thread changing the value so it would not have any safety issues. I think it should be clear a static will not work well for this from the last previous two paragraphs. As an unrelated aside, if we had 1$ for every static field we have used which ended up pinning some part of our runtime from unloading I would have enough for a nice lunch and a couple of drinks :) [Additional Note: @subbuss rightly points that this feels like a smell in our codebase in IRRuntimeHelpers.updateBlockState(). Unfortunately all of our block code feels like a smell :) The main consumer of this info is IRRuntimeHelpers.updateBlockState which determines proper self. Sort of feels like we are digging this out of a struct which changes per thread when if we passed it around through the call paths we would just know what it was. No doubt we did not make that change because it would end up bubbling through many methods....Sorry for the rambling and I appreciate any thoughts on this] |
thanks @enebo seems like solving the leak for good might take some. thought its not the best code when I setup the PR :) but should at least make logs very less verbose (and use less thread-local storage) since NONE is the absolute top value. I won't be looking into a real leak solution here anytime soon, if anyone is for it feel free to grab my pieces and close the PR. |
@kares yeah it seems you are helping/reducing the obvious immediate problem and that is good. |
@enebo should be fine to go, at least as an improvement in managing very noisy TC logs, for 9.1.6.0 ? |
instead treat
null
asNONE
this is more of a cosmetic issue where on Tomcat JRuby 9K prints a lot of SEVERE warning due Tomcat's leak detection :