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

[Stefan Karlsson]

On 2014-06-26 16:16, 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