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

[Christian Thalinger]

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

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