Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Application failure after updating to Java 7u40 with invokedynamic #1005

Closed
donv opened this Issue · 15 comments

5 participants

@donv
Collaborator

Hi all!

On one of our airport applications we tried updating to Java 7u40, and all hell broke loose. The most noticeable symptom is that columns added by ActiveRecord migrations are not recognised later in the app.

This has been tested with JRuby 1.7.4 and 1.7.5.dev (2013-09-11), on Linux, Windows, and OS X.

Turning off InvokeDynamic restores normal behaviour.

I thought I'd ask if there are known issues with InvokeDynamic and/or Java 7u40 before spending time on reproduction.

Are there any known issues with JRuby and InvokeDynamic and/or Java 7u40?

@BanzaiMan
Owner

Could this be a different manifestation of the problem with 7u40 and/or InvokeDynamic? (#976)

@alno

Maybe related to this bug.

Using 1.7.4. After updating to 7u40 today I got unstable problem during reloading model:

NoMethodError - super: no superclass method `inherited_without_kaminari':
  org/jruby/RubyBasicObject.java:1696:in `method_missing'
  activerecord (4.0.0) lib/active_record/dynamic_matchers.rb:22:in `method_missing'
  activerecord (4.0.0) lib/active_record/core.rb:96:in `inherited'
  kaminari (0.14.1) lib/kaminari/models/active_record_extension.rb:10:in `inherited_with_kaminari'
  app/models/region_activity.rb:1:in `(root)'
  org/jruby/RubyKernel.java:1073:in `load'
  activesupport (4.0.0) lib/active_support/dependencies.rb:423:in `load_file'
  activesupport (4.0.0) lib/active_support/dependencies.rb:615:in `new_constants_in'
  activesupport (4.0.0) lib/active_support/dependencies.rb:614:in `new_constants_in'
  activesupport (4.0.0) lib/active_support/dependencies.rb:422:in `load_file'
@headius
Owner

@alno: The super issue is fixed in 1.7.5, but we'd appreciate if you can confirm that by testing against JRuby master.

@headius
Owner

@donv Do you have more specific details on the issues that arose?

@headius
Owner

@donv I'm very interested in getting your app up and working under invokedynamic and fixing all issues we find for 1.7.5. I'll arrive in Barcelona in a few hours and check in from the hotel.

@donv
Collaborator

Hi @headius !

I am working on it now. Should I try running with JRuby master? Using RVM?

@donv
Collaborator

Hi @headius !

I am working on this now. I'll post the current error symptoms, and try to reduce the test case.

@headius
Owner

Yes please try JRuby master. I'll be online the rest of today.

@donv
Collaborator

I'll be online most of the evening and night if necessary. If you could look at the example, it would be great.

@headius
Owner

Looking at this now.

@headius
Owner

Ok, I finally have an explanation for this one.

Here's a reproduction that I'll use to discuss the problem: https://gist.github.com/headius/6599215

So the issue here is how the @@ variables are accessed.

@@var class vars are loaded using the current static scope (lexical scope from parser) which contains a reference to the surrounding class at the time the code was instantiated. The scope itself gets stored either in the AST (interpreter) or in the AbstractScript associated with the compiled code. When retrieving an class variable, we first acquire a reference to the scope and then look up the variable against the class it references.

In the non-indy compiler, the scope is retrieved each time from the AbstractScript object associated with the currently executing code. This requires hopping through a couple levels of indirection, but if the same JVM bytecode is used with different logical code bodies, they continue to work properly since they always go to the AbstractScript object.

The situation with indy is a bit different.

In order to make it possible for the JVM JIT to fold away repeated accesses of the same scope, we permanently cache the scope at the physical call site. The call site is unique to a given loaded piece of bytecode, so if the same loaded bytecode gets shared across logically different code bodies, we can end up using the scope associated with a previously jitted version of the same code. That's what's happening here.

In my example, the class variable accessors for @@bar are eval'ed into multiple classes, but to our JIT cache they look identical since they don't include anything to make them appear unique for each class. This means we use a single loaded version of the bytecode and all subclasses end up sharing the same bytecode. This means, in turn, they share a single call site...and the hard caching of the scope ends up going to the first method to JIT. In this case, that means FooAlpha's bar and @@bar are used rather than FooBeta's when the latter's bar methods JIT.

The core problem here is the caching for JVM bytecode/class for a JITed method does not have a sufficiently unique key to represent identical bytecode that has different runtime artifacts attached to it...like static scope.

There are a few ways to fix this:

  • Modify the static scope logic in our invokedynamic backend to not permanently cache the scope. This would work, but would cause repeat accesses of that scope to go through full indirection every time. It may require also fixing other values looked up from scope like constants...and losing JIT value folding for constants would be a terrible step backward.

  • Modify the key for the JIT cache to include additional information about the lexical scoping structures being used. This may require some help from the parser, since at the moment there's no way to distinguish two StaticScope objects other than by their hashcodes. Adding the scope hashcodes to the key would be a quick way to fix this without completely breaking caching, but it may be that bytecode caching in the presence of invokedynamic will bite us in other ways down the road.

  • Disable JIT caching in the presence of invokedynamic. The feature was originally added when it was assumed that all users would always need to run multiple copies of the same JRuby application in a given JVM, where we would potentially crumble under the load of all those mostly-identical JITed methods. However, applications have been trending toward a single JRuby instance across all threads, memory has been getting cheaper, and Java 8 eliminates the permgen we were originally so concerned about.

I'm leaning strongly enough toward the third option that I'm going to go with it for now. So many of our uses of invokedynamic require the loaded code and call sites to be as unique as the logical methods they represent, and I can think of several other places where the current caching could (and maybe already is) biting us. This will potentially mean JRuby apps running under indy load more code, but in actuality it will almost exclusively affect multi-instance applications. Provide the JVM with more memory or run a single instance.

@headius headius closed this issue from a commit
@headius headius Disable the JIT cache when invokedynamic is enabled.
This fixes #1005.

Here's a reproduction that I'll use to discuss the problem:
https://gist.github.com/headius/6599215

So the issue here is how the @@ variables are accessed.

@@var class vars are loaded using the current static scope (lexical scope from
parser) which contains a reference to the surrounding class at the time the
code was instantiated. The scope itself gets stored either in the AST
(interpreter) or in the AbstractScript associated with the compiled code. When
retrieving an class variable, we first acquire a reference to the scope and
then look up the variable against the class it references.

In the non-indy compiler, the scope is retrieved each time from the
AbstractScript object associated with the currently executing code. This
requires hopping through a couple levels of indirection, but if the same JVM
bytecode is used with different logical code bodies, they continue to work
properly since they always go to the AbstractScript object.

The situation with indy is a bit different.

In order to make it possible for the JVM JIT to fold away repeated accesses of
the same scope, we permanently cache the scope at the physical call site. The
call site is unique to a given loaded piece of bytecode, so if the same loaded
bytecode gets shared across logically different code bodies, we can end up
using the scope associated with a previously jitted version of the same code.
That's what's happening here.

In my example, the class variable accessors for @@bar are eval'ed into multiple
classes, but to our JIT cache they look identical since they don't include
anything to make them appear unique for each class. This means we use a single
loaded version of the bytecode and all subclasses end up sharing the same
bytecode. This means, in turn, they share a single call site...and the hard
caching of the scope ends up going to the first method to JIT. In this case,
that means FooAlpha's bar and @@bar are used rather than FooBeta's when the
latter's bar methods JIT.

The core problem here is the caching for JVM bytecode/class for a JITed method
does not have a sufficiently unique key to represent identical bytecode that
has different runtime artifacts attached to it...like static scope.

There are a few ways to fix this:

Modify the static scope logic in our invokedynamic backend to not permanently
cache the scope. This would work, but would cause repeat accesses of that scope
to go through full indirection every time. It may require also fixing other
values looked up from scope like constants...and losing JIT value folding for
constants would be a terrible step backward.

Modify the key for the JIT cache to include additional information about the
lexical scoping structures being used. This may require some help from the
parser, since at the moment there's no way to distinguish two StaticScope
objects other than by their hashcodes. Adding the scope hashcodes to the key
would be a quick way to fix this without completely breaking caching, but it
may be that bytecode caching in the presence of invokedynamic will bite us in
other ways down the road.

Disable JIT caching in the presence of invokedynamic. The feature was
originally added when it was assumed that all users would always need to run
multiple copies of the same JRuby application in a given JVM, where we would
potentially crumble under the load of all those mostly-identical JITed methods.
However, applications have been trending toward a single JRuby instance across
all threads, memory has been getting cheaper, and Java 8 eliminates the permgen
we were originally so concerned about.

I'm leaning strongly enough toward the third option that I'm going to go with
it for now. So many of our uses of invokedynamic require the loaded code and
call sites to be as unique as the logical methods they represent, and I can
think of several other places where the current caching could (and maybe
already is) biting us. This will potentially mean JRuby apps running under indy
load more code, but in actuality it will almost exclusively affect
multi-instance applications. Provide the JVM with more memory or run a single
instance.
8edcc26
@headius headius closed this in 8edcc26
@donv
Collaborator

Thank you! It works perfectly in the test case and in our app. Excellent!

One observation though:

The given test case with AircraftType and BlacklistedCard loading from a YAML file takes around 40 seconds with indy disabled and over 80 seconds with indy enabled, all other settings equal.

I assume this is not the intended effect of activating indy :) Is this a known issue or should I file it?

@hosamaly

Thanks for your efforts! Is it possible to back-port this change to JRuby 1.7.4?

@BanzaiMan
Owner

@hosamaly 1.7.4 has already been released, so there is no "fixing" it. 1.7.5 will be released soon, so please use that.

@hosamaly

@BanzaiMan But JRuby 1.7.5 has 200 resolved issues, any of which could have introduced a regression issue or an incompatibility with existing code. This is not to undermine the quality of development, but these issues can happen. The development community needs time to assert the status of 1.7.5 as being stable. So I was hoping to make something like 1.7.4.1 that would include just this fix, without forcing us to go with a new release whose stability is not yet guaranteed..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.