On Aug 9, 2011, at 4:33 AM, Christian Thalinger wrote:
>
> 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
Yes that right. The new webrev looks good.
tom
>
> ! 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)
>>>
>>
>
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
