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

[Stefan Karlsson]

On 2014-06-25 23:53, Igor Veresov wrote:
> Looks good. Very nice improvement.

Thanks, Igor!

>
> igor
>
> ps. We should probably get something like a BOT for the CodeCache. The reason RelocIterator's constructor is so bad is that it actually iterates over the whole code cache to find the blob that contains the start address. Ugh..

Actually, in the code I change we already have the nm setup, so we don't 
have to search the code cache with:
   if (nm == NULL && begin != NULL) {
     // allow nmethod to be deduced from beginning address
     CodeBlob* cb = CodeCache::find_blob(begin);
     nm = cb->as_nmethod_or_null();
   }

I think that the problem is that set_limits start from 
relocation_begin() and searches it's way forward to the relocInfo we 
want to visit, given the 'begin' parameter.

     while (true) {
       backup      = _current;
       backup_addr = _addr;
#ifdef ASSERT
       if (backup == infoCheck) {
         assert(backup_addr == addrCheck, "must match"); addrCheck = 
NULL; infoCheck = NULL;
       } else {
         assert(addrCheck == NULL || backup_addr <= addrCheck, "must not 
pass addrCheck");
       }
#endif // ASSERT
       if (!next() || addr() >= begin) break;
     }

Since we already had the correct relocInfo in the original 
relocIterator, this is unnecessary work.

thanks,
StefanK

>
>
> On Jun 24, 2014, at 6:52 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>
>> 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
>>>>