On Aug 10, 2011, at 9:28 PM, Tom Rodriguez wrote:
>
> 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.
Thank you, Tom.
-- Christian
>
> 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
