Skip to content
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.

Sign up for GitHub

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

Jump to bottom

do not eagerly set eval-type in a thread-local #4215

Merged
merged 1 commit into from Nov 8, 2016
Merged

do not eagerly set eval-type in a thread-local #4215

merged 1 commit into from Nov 8, 2016

Conversation

kares
Copy link
Member

@kares kares commented Oct 10, 2016

instead treat null as NONE

this is more of a cosmetic issue where on Tomcat JRuby 9K prints a lot of SEVERE warning due Tomcat's leak detection :

04-Oct-2016 11:45:01.304 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@1311e832]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.304 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@7ca33465]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@3ec512d0]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@7c5635e1]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@7feb868c]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@3fce92f0]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@1310eb4a]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@3bc7510d]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@1a9a4b24]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
04-Oct-2016 11:45:01.305 SEVERE [localhost-startStop-2] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [sblending] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@7bb3621]) and a value of type [org.jruby.EvalType] (value [NONE]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
...
... (400 lines of the same on a booted mid-size Rails app)

@kares kares added this to the JRuby 9.1.6.0 milestone Oct 10, 2016
@kares kares mentioned this pull request Oct 10, 2016
Closed
enebo
enebo requested changes Oct 10, 2016
View reviewed changes
Copy link
Member

@enebo enebo left a 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?

@kares
Copy link
Member Author

kares commented Oct 10, 2016

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.

@enebo
Copy link
Member

enebo commented Oct 10, 2016

@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.

@kares
Copy link
Member Author

kares commented Oct 10, 2016

@enebo not sure but I think in that case the thread-local instance sharing is essential - changing to getEvalType could end up being a bug (since the instance would no longer be shared).

@darmbrust
Copy link

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
public void setEvalType(EvalType evalType)
it is followed up with another set later, that returns it to null?

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.

@kares
Copy link
Member Author

kares commented Oct 11, 2016

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 teardown JRuby could clear the thread-local (since there would be one predictable place to do so e.g. org.jruby.runtime.EvalType.current) but that would mean block bodies would need to pro-actively set the thread-local whenever they are used, right @enebo ?

@darmbrust
Copy link

Is this class a singleton in its usage pattern? Or is it created per request?

@enebo
Copy link
Member

enebo commented Oct 11, 2016

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]

@kares
Copy link
Member Author

kares commented Oct 12, 2016

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.

@enebo
Copy link
Member

enebo commented Oct 12, 2016

@kares yeah it seems you are helping/reducing the obvious immediate problem and that is good.

@kares
Copy link
Member Author

kares commented Nov 8, 2016

@enebo should be fine to go, at least as an improvement in managing very noisy TC logs, for 9.1.6.0 ?

@enebo enebo merged commit b23d4ad into jruby:master Nov 8, 2016
@kares kares mentioned this pull request Mar 14, 2017
Closed
kares added a commit that referenced this pull request Mar 14, 2017
@kares
also treat NONE eval-type as a (thread-local) null when its set
3a88f09 
this is a follow-up on 0badff8 (from #4215)

resolves #4529
@kares kares mentioned this pull request Mar 14, 2017
Merged
kares added a commit that referenced this pull request Mar 14, 2017
@kares
also treat NONE eval-type as a (thread-local) null when its set
c1da2a3 
this is a follow-up on 0badff8 (from #4215)

resolves #4529
@kares kares deleted the test-null-eval-type branch March 15, 2017 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enebo enebo enebo requested changes

Assignees
No one assigned
Labels
enhancement ir JRuby 9000
Projects
None yet
Milestone
JRuby 9.1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kares @enebo @darmbrust