RFR (XL): 8152664 - Support non-continuous CodeBlobs in HotSpot

[Rickard Bäckman]

Thank you for the review Igor.

/R

On 04/22, Igor Veresov wrote:
> This looks good to me.
> 
> igor
> 
> 
> > On Apr 20, 2016, at 12:43 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
> > 
> > Volker,
> > 
> > sorry about the confusion. To me the only thing that made sense was to
> > make the push of the two commits atomically. I didn't intend to include
> > your changes in the webrev but I forgot to pop the changes of my mq.
> > 
> > I hope you got the answers on some of the oddities from Christian. But
> > to add to that. The reason we have NMethodIterator and
> > CompiledMethodIterator is that we sometimes want to make sure we only
> > iterate over nmethods. Hope that works.
> > 
> > Here is an updated webrev with the other changes you commented on:
> > 
> > http://cr.openjdk.java.net/~rbackman/8152664.4/
> > 
> > Thanks
> > /R
> > 
> > On 04/19, Volker Simonis wrote:
> >> Hi Rickard,
> >> 
> >> I just wanted to prepare the new webrev for 8151956 but I'm a little
> >> confused because I realized that your latest webrev already contains the
> >> changes which I had proposed for 8151956.
> >> 
> >> But after thinking about it a little bit I think that's fine. If I prepare
> >> a patch for 8151956 which is intended to be pushed BEFORE 8152664 you'd had
> >> to adapt 8152664 to take care of the new changes introduced by 8151956. If
> >> I prepare a patch for 8151956 which is intended to be pushed AFTER 8152664
> >> it would be hard to review it (because it will depend on 8152664) and we
> >> would get a change in the repo which would not build on PPC64 and AARCH64
> >> which isn't nice either.
> >> 
> >> So altogether I think it's fine to incorporate the fix for 8151956 into
> >> your change. Please only don't forget to close 8151956 as "fixed by
> >> 8152664" after you have pushed the changes for 8152664.
> >> 
> >> I've verified that your last webrev builds and runs fine on Linux/ppc64 and
> >> AIX. You've also fixed all the issues I've addressed in my first mail to
> >> this thread and the typo in os_linux_aarch64.cpp found by Andrew - thanks!
> >> 
> >> Some final nit-picking:
> >> 
> >> - you still have the white-space only change in os_windows.cpp objected by
> >> Vladimir.
> >> 
> >> - in codeBlob.cpp can you please update the following comments to reflect
> >> the new types:
> >> 
> >>  // Creates a simple CodeBlob. Sets up the size of the different
> >> regions.*  CodeBlob::CodeBlob(const char* name, int header_size, int
> >> size, int frame_complete, int locs_size) {**    assert(size        ==
> >> round_to(size,        oopSize), "unaligned size");**+
> >> RuntimeBlob::RuntimeBlob(const char* name, int header_size, int size,
> >> int frame_complete, int locs_size)*
> >> 
> >>  // Creates a CodeBlob from a CodeBuffer. Sets up the size of the
> >> different regions,  // and copy code and relocation info.*!
> >> CodeBlob::CodeBlob(**! RuntimeBlob::RuntimeBlob(*
> >> 
> >> 
> >> - why do we need:
> >> 
> >> *+   bool  make_not_used()    { return make_not_entrant(); }*
> >> 
> >> it only forwards to make_not_entrant() and it is only used a single time in
> >> ciEnv.cpp:
> >> 
> >> *!             old->make_not_entrant();**!             old->make_not_used();*
> >> 
> >> 
> >> - I don't understand why we need both NMethodIterator and
> >> CompiledMethodIterator - they're virtually the same and nmethod is
> >> currently the only subclass of CompiledMethod. Can you please be more
> >> specific why you've changed some instances of NMethodIterator to
> >> CompiledMethodIterator and others not. Without background information this
> >> makes no sense to me. Also, the advance method in CompiledMethodIterator
> >> isn't "inline" while the one in NMethodIterator is - don't know if this
> >> will be a performance problem.
> >> 
> >> The rest looks good to me but please notice that I still haven't looked at
> >> all changes (especially not on the agent/ and dtrace/ files). So you should
> >> get at least one more reviewer for such a big change.
> >> 
> >> Regards,
> >> Volker
> >> 
> >> 
> >> 
> >> On Tue, Apr 19, 2016 at 7:32 AM, Rickard Bäckman <rickard.backman at oracle.com
> >>> wrote:
> >> 
> >>> Here is the updated webrev, rebased and I think I have fixed all the
> >>> comments with one exception.
> >>> 
> >>> I've avoided making CompiledMethodIterator and NMethodIterator a
> >>> template class for now. I agree we should do something to reuse the
> >>> parts that are identical but for now I think there will be a few more
> >>> changes to CompiledMethodIterator in an upcoming RFR. So can we hold off
> >>> with that change?
> >>> 
> >>> Webrev: http://cr.openjdk.java.net/~rbackman/8152664.3/
> >>> 
> >>> Thanks
> >>> 
> >>> On 04/07, Rickard Bäckman wrote:
> >>>> Hi,
> >>>> 
> >>>> can I please have review for this patch please?
> >>>> 
> >>>> So far CodeBlobs have required all the data (metadata, oops, code, etc)
> >>>> to be in one continuous blob With this patch we are looking to change
> >>>> that. It's been done by changing offsets in CodeBlob to addresses,
> >>>> making some methods virtual to allow different behavior and also
> >>>> creating a couple of new classes. CompiledMethod now sits inbetween
> >>>> CodeBlob and nmethod.
> >>>> 
> >>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152664
> >>>> Webrev: http://cr.openjdk.java.net/~rbackman/8152664/
> >>>> 
> >>>> Thanks
> >>>> /R
> >>> /R
> >>> 
>