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

[Vladimir Kozlov]

Christian Thalinger wrote:
> On Aug 8, 2011, at 4:55 PM, Vladimir Kozlov wrote:
> 
>> Christian,
>>
>> Should we put "skip bytecode quickening" code under flag to do this only when invoke dynamic is enabled? Or put_code is zero only in invoke dynamic case?
> 
> No, it doesn't buy us anything.  The new checking code is only executed the first time as the bytecodes are quickened right after that.  And in the case where a putfield isn't quickened and we call resolve_get_put it gets very expensive anyway.

You lost me here. New code in resolve_get_put() is executed only for putfield to 
CallSite.target. But new code in patch_bytecode() skips quickening for all 
putfield bytecodes. My question is: can you narrow skipping quickening only for 
putfield to CallSite.target? Or you are saying that there is no performance 
difference between executing _aputfield vs _fast_aputfield?

Vladimir

> 
>> On 8/8/11 6:56 AM, Christian Thalinger wrote:
>>>> Why on sparc you use ld_ptr() to load from cache but on X86 and X64 you use movl() (only 32  bit)?
>>> Good question.  I took the code from TemplateTable::resolve_cache_and_index without thinking about it and that one uses ld_ptr.
>>>
>>> _indices in CosntantPoolCacheEntry is defined as intx:
>>>
>>>   volatile intx     _indices;  // constant pool index&  rewrite bytecodes
>>>
>>> and bytecode 1 and 2 are in the upper 16-bit of the lower 32-bit word:
>>>
>>> // bit number |31                0|
>>> // bit length |-8--|-8--|---16----|
>>> // --------------------------------
>>> // _indices   [ b2 | b1 |  index  ]
>>>
>>> Loading 32-bit on LE gives you the right bits but on BE it does not.  I think that's the reason for the "optimization" on x64.
>> I don't like this "optimization" but I understand why we using it. Add a comment (especially in x64 file).
> 
> I factored reading the bytecode into InterpreterMacroAssembler::get_cache_and_index_and_bytecode_at_bcp since the same code is used twice in TemplateTable and added the comment there.
> 
>>>> I am concern about using next short branch in new code in templateTable_sparc.cpp:
>>>>
>>>> cmp_and_br_short(..., L_patch_done);  // don't patch
>>>>
>>>> There is __ stop() call which generates a lot of code so that label L_patch_done could be far.
>>> Yeah, I thought I give it a try if it works.  cmp_and_br_short should assert if the branch displacement is too far, right?
>>>
>> Yes, it will assert but may be only in some worst case which we do not test. For example, try to run 64 bit fastdebug VM on Sparc + compressed oops + VerifyOops.
> 
> That works.
> 
>>>>
>>>> Why you added new #include into ciEnv.cpp and nmethod.cpp, what code needs it? Nothing else is changed in these files.
>>> Both files use dependencies and I got linkage errors on Linux while working on the fix (because of inline methods).  It seems that the include is not required in ciEnv.cpp because ciEnv.hpp already includes it.  I missed that.  But nmethod.cpp needs it because nmethod.hpp only declares class Dependencies.
>>>
>> OK.
>>
>>>> Why you did not leave "volatile" call site inlining with guard? You did not explain why virtual call is fine for it.
>>> The spec of MutableCallSite says:
>>>
>>> "For target values which will be frequently updated, consider using a volatile call site instead."
>>>
>>> And VolatileCallSite says:
>>>
>>> "A VolatileCallSite is a CallSite whose target acts like a volatile variable. An invokedynamic instruction linked to a VolatileCallSite sees updates to its call site target immediately, even if the update occurs in another thread. There may be a performance penalty for such tight coupling between threads.
>>>
>>> Unlike MutableCallSite, there is no syncAll operation on volatile call sites, since every write to a volatile variable is implicitly synchronized with reader threads.
>>>
>>> In other respects, a VolatileCallSite is interchangeable with MutableCallSite."
>>>
>>> Since VolatileCallSite really should only be used when you know the target changes very often we don't do optimizations for this case.  Obviously this is just a guess how people will use VolatileCallSite but I think for now this is a safe bet.
>>>
>> Thank you for explaining it.
>>
>>> Additionally I had to do two small changes because the build was broken on some configurations:
>>>
>>> -  klassOop new_type = _changes.is_klass_change() ? _changes.as_klass_change()->new_type() : NULL;
>>> +  klassOop new_type = _changes.is_klass_change() ? _changes.as_klass_change()->new_type() : (klassOop) NULL;
>>>
>>> and
>>>
>>> -      MutexLockerEx ccl(CodeCache_lock, thread);
>>> +      MutexLockerEx ccl(CodeCache_lock, Mutex::_no_safepoint_check_flag);
>>>
>>> I updated the webrev.
>> Good.
> 
> Thanks.
> 
> -- Christian
> 
>> Vladimir
>>
>>> -- Christian
>>>
>>>>
>>>> Vladimir
>>>>
>>>> On 8/5/11 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)
>>>>>
>