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

[Volker Simonis]

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
>