RFR: 8047326: Add a version of CompiledIC_at that doesn't create a new RelocIterator

[Stefan Karlsson]

Hi all,

Could someone from the Compiler team take a look and review/comment on 
this patch?

thanks,
StefanK

On 2014-06-19 17:36, Stefan Karlsson wrote:
> This was meant for the hotspot-dev list. BCC:ing hotspot-gc-dev.
>
> On 2014-06-19 14:45, Stefan Karlsson wrote:
>> Hi all,
>>
>> I have a patch that we have been using in the G1 Class Unloading 
>> project to lower the remark times.  This changes Compiler code, so I 
>> would like to get feedback from the Compiler team.
>>
>> http://cr.openjdk.java.net/~stefank/8047362/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8047362
>>
>> The patch builds upon the patch in:
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-June/014358.html
>>
>>
>> Summary from the bug report:
>> ---
>> Creation of RelocIterators show up high in profiles of the remark 
>> phase, in the G1 Class Unloading project.
>>
>> There's a pattern in the nmethod/codecache code to create a 
>> RelocIterator and then materialize a CompiledIC:
>>
>>     RelocIterator iter(this, low_boundary);
>>     while(iter.next()) {
>>       if (iter.type() == relocInfo::virtual_call_type) {
>>         CompiledIC *ic = CompiledIC_at(iter.reloc());
>>
>> CompiledIC_at is implemented as:
>>   new CompiledIC(call_site->code(), nativeCall_at(call_site->addr()));
>>
>> And one of the first thing CompiledIC::CompiledIC(const nmethod* nm, 
>> NativeCall* call) does is to create a new RelocIterator:
>> ...
>> address ic_call = call->instruction_address();
>> ...
>>   RelocIterator iter(nm, ic_call, ic_call+1);
>>   bool ret = iter.next();
>>   assert(ret == true, "relocInfo must exist at this address");
>>   assert(iter.addr() == ic_call, "must find ic_call");
>>
>> I would like to propose that we pass down the RelocIterator that we 
>> already have, instead of creating a new.
>> ---
>>
>>
>> I've previously received feedback that this seems like reasonable 
>> thing to do, but that the parameter to the new CompileIC_at should 
>> take a const RelocIterator* instead of RelocIterator*. I couldn't do 
>> that without changing a significant amount of Compiler code, so I 
>> have left it out for now. Any opinions on how to handle that?
>>
>>
>> To give an idea of the performance difference, I temporarily added 
>> the following code:
>> void CodeCache::iterate_through_CIs(int style) {
>>   int count;
>>   FOR_ALL_ALIVE_NMETHODS(nm) {
>>     RelocIterator iter(nm);
>>     while(iter.next()) {
>>       if (iter.type() == relocInfo::virtual_call_type ||
>>           iter.type() == relocInfo::opt_virtual_call_type) {
>>         if (style > 0) {
>>           CompiledIC *ic = style == 1 ? CompiledIC_at(&iter) : 
>> CompiledIC_at(iter.reloc());
>>           if (ic->ic_destination() == (address)0xdeadb000) {
>>             gclog_or_tty->print_cr("ShouldNotReachHere");
>>           }
>>         }
>>       }
>>     }
>>   }
>> }
>>
>> and then measured how long time it took to execute 
>> iterate_through_CIs(style) 1000 times with style == {0, 1, 2}.
>>
>> The results are:
>>  iterate_through_CIs(0): 1.210833 s // No CompiledICs created
>>  iterate_through_CIs(1): 1.976557 s // New style
>>  iterate_through_CIs(2): 9.924209 s // Old style
>>
>>
>> Testing:
>>   A similar version has been used and thoroughly been tested together 
>> with the other G1 Class Unloading changes. This exact version has so 
>> far only been tested with Kitchensink and SpecJVM2008 
>> compiler.compiler. What test lists would be appropriate to test this 
>> with?
>>
>>
>> thanks,
>> StefanK
>>
>