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

[Christian Thalinger]

> On Apr 19, 2016, at 11:35 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> 
> On Tue, Apr 19, 2016 at 6:49 PM, Christian Thalinger <christian.thalinger at oracle.com <mailto:christian.thalinger at oracle.com>> wrote:
> 
>> On Apr 19, 2016, at 4:30 AM, Volker Simonis <volker.simonis at gmail.com <mailto: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 totally agree with you; it’s a double-standard and it’s frustrating.  Believe me, if it were up to me the AOT implementation would be open-source.  But I truly believe that the implementation will be opened up in a future JDK release.

In general, it’s a nice refactoring and abstraction and could possibly help other projects like Sumatra if we ever get back to it.

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

Conversely, if you have changes you want to get it you can use this as a precedent :-)

>  
>> 
>> 
>> - 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 <mailto: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/ <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 <https://bugs.openjdk.java.net/browse/JDK-8152664>
>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8152664/ <http://cr.openjdk.java.net/~rbackman/8152664/>
>>>> 
>>>> Thanks
>>>> /R
>>> /R