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

[Stefan Karlsson]

On 2014-06-26 16:18, Mikael Gerdin wrote:
> I replied to the wrong list, sorry.
> Forwarding my review to hotspot-dev.
>
> /Mikael
>
> On Thursday 26 June 2014 16.16.36 Mikael Gerdin wrote:
>> Hi,
>>
>> On Thursday 19 June 2014 17.36.44 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/
>> The change looks good.
>>
>> I had an offline discussion with Steafan about this and we think that it
>> would actually suffice to pass down the Relocation* since it appears to
>> contain all the information needed to create the CompiledIC objects.
>> However in the interest of moving forward with changes built on top of this
>> we will look at that for a future cleanup.

Thanks.

StefanK

>>
>> /Mikael
>>
>>>> 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