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

[Rickard Bäckman]

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