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

[Christian Thalinger]

On Aug 8, 2011, at 8:49 PM, Tom Rodriguez wrote:

> 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
> }

I see your point.  But the code above is broken as changes->method_handle() will not work when changes == NULL.  One of my first versions of this code also stored the MethodHandle target in the dependence stream which seems to be required when we want to validate all CallSites.  Something like this:

! klassOop Dependencies::check_call_site_target_value(klassOop ctxk, oop call_site, oop method_handle, CallSiteDepChange* changes) {
+   assert(call_site    ->is_a(SystemDictionary::CallSite_klass()),     "sanity");
+   assert(method_handle->is_a(SystemDictionary::MethodHandle_klass()), "sanity");
+   if (changes == NULL) {
+     // Validate all CallSites
+     if (java_lang_invoke_CallSite::target(call_site) != method_handle)
+       return ctxk;  // assertion failed
+   } else {
+     // Validate the given CallSite
+     if (call_site == changes->call_site() && java_lang_invoke_CallSite::target(call_site) != changes->method_handle()) {
+       assert(method_handle != changes->method_handle(), "must be");
+       return ctxk;  // assertion failed
+     }
+   }
+   assert(java_lang_invoke_CallSite::target(call_site) == 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.

Where it says:

// %%% The Universe::flush_foo methods belong in CodeCache.

:-)

>   Also add some comments explaining why it's doing what it's doing.

Done.

> 
> doCall.cpp:
> 
> Can you put in a comment explaining that VolatileCallSite is never inlined.

Done.

> 
> Otherwise it looks good.

webrev updated.

-- Christian

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