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

Merged
merged 1 commit into from Nov 8, 2016

Conversation

Projects
None yet
3 participants
@kares
Member

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)
@enebo

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 10, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 10, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 10, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@darmbrust

darmbrust Oct 10, 2016

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.

darmbrust commented Oct 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 11, 2016

Member

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 ?

Member

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

This comment has been minimized.

Show comment
Hide comment
@darmbrust

darmbrust Oct 11, 2016

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

darmbrust commented Oct 11, 2016

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

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 11, 2016

Member

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]

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 12, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 12, 2016

Member

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

Member

enebo commented Oct 12, 2016

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

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Nov 8, 2016

Member

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

Member

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

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

kares added a commit that referenced this pull request Mar 14, 2017

kares added a commit that referenced this pull request Mar 14, 2017

@kares kares deleted the kares:test-null-eval-type branch Mar 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment