Request for review (L): 7071653: JSR 292: call site change notification should be pushed not pulled

[Tom Rodriguez]

dependencies.cpp:

in check_call_site_target_value, the changes == NULL case should be checking that the call site hasn't changed.  It should probably look more like this:

klassOop Dependencies::check_call_site_target_value(klassOop ctxk, oop call_site, CallSiteDepChange* changes) {
  assert(call_site->is_a(SystemDictionary::CallSite_klass()), "sanity");
  // Same CallSite object but different target?  Check this specific call site
  //  if changes is non-NULL or validate all CallSites
  if ((changes == NULL || (call_site == changes->call_site())) &&
      (java_lang_invoke_CallSite::target(call_site) != changes->method_handle())) {
    return ctxk;  // assertion failed
  }
  assert(java_lang_invoke_CallSite::target(call_site) == changes->method_handle(), "should still be valid");
  return NULL;  // assertion still valid
}

The final assert is just a paranoia check that a call site hasn't changed without the dependencies being checked.

interpreterRuntime.cpp:

Please move the dependence check code into universe with the other dependence check code.  Also add some comments explaining why it's doing what it's doing.

doCall.cpp:

Can you put in a comment explaining that VolatileCallSite is never inlined.

Otherwise it looks good.

tom


On Aug 5, 2011, at 6:32 AM, Christian Thalinger wrote:

> http://cr.openjdk.java.net/~twisti/7071653
> 
> 7071653: JSR 292: call site change notification should be pushed not pulled
> Reviewed-by:
> 
> Currently every speculatively inlined method handle call site has a
> guard that compares the current target of the CallSite object to the
> inlined one.  This per-invocation overhead can be removed if the
> notification is changed from pulled to pushed (i.e. deoptimization).
> 
> I had to change the logic in TemplateTable::patch_bytecode to skip
> bytecode quickening for putfield instructions when the put_code
> written to the constant pool cache is zero.  This is required so that
> every execution of a putfield to CallSite.target calls out to
> InterpreterRuntime::resolve_get_put to do the deoptimization of
> depending compiled methods.
> 
> I also had to change the dependency machinery to understand other
> dependencies than class hierarchy ones.  DepChange got the super-type
> of two new dependencies, KlassDepChange and CallSiteDepChange.
> 
> Tested with JRuby tests and benchmarks, hand-written testcases, JDK
> tests and vm.mlvm tests.
> 
> Here is the speedup for the JRuby fib benchmark (first is JDK 7 b147,
> second with 7071653).  Since the CallSite targets don't change during
> the runtime of this benchmark we can see the performance benefit of
> eliminating the guard:
> 
> $ jruby --server bench/bench_fib_recursive.rb 5 35
>  0.883000   0.000000   0.883000 (  0.854000)
>  0.715000   0.000000   0.715000 (  0.715000)
>  0.712000   0.000000   0.712000 (  0.712000)
>  0.713000   0.000000   0.713000 (  0.713000)
>  0.713000   0.000000   0.713000 (  0.712000)
> 
> $ jruby --server bench/bench_fib_recursive.rb 5 35
>  0.772000   0.000000   0.772000 (  0.742000)
>  0.624000   0.000000   0.624000 (  0.624000)
>  0.621000   0.000000   0.621000 (  0.621000)
>  0.622000   0.000000   0.622000 (  0.622000)
>  0.622000   0.000000   0.622000 (  0.621000)
>