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

[Mikael Gerdin]

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