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

[Volker Simonis]

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