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

[Rickard Bäckman]

Volker, 

are you ok with the last changes?

http://cr.openjdk.java.net/~rbackman/8152664.4/

Thanks

On 04/19, Volker Simonis wrote:
> On Tue, Apr 19, 2016 at 6:49 PM, Christian Thalinger <
> christian.thalinger at oracle.com> wrote:
> 
> >
> > On Apr 19, 2016, at 4:30 AM, Volker Simonis <volker.simonis at gmail.com>
> > 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 can answer this.  make_not_used is virtual:
> >
> >   virtual bool make_not_used() = 0;
> >
> > Can you guess why this is the case? :-)  The reason is that the
> > implementation is different for AOT compiled methods.
> >
> >
> OK, I see. Thanks for the background info but now I can not refrain from
> commenting :)
> 
> If SAP (or anybody else outside Oracle) would submit such a kind of XL
> change in order to better support let's say it's closed HPUX/Itanium port I
> don't think it would be even considered.
> 
> I don't want to reject these specific change (I came to terms with it :)
> but I think this should stand as bad example for changes which will not
> happen too often in the future.
> 
> 
> >
> >
> > - 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
> >
> >
> >