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

[Igor Veresov]

This looks good to me.

igor


> On Apr 20, 2016, at 12:43 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
> 
> Volker,
> 
> sorry about the confusion. To me the only thing that made sense was to
> make the push of the two commits atomically. I didn't intend to include
> your changes in the webrev but I forgot to pop the changes of my mq.
> 
> I hope you got the answers on some of the oddities from Christian. But
> to add to that. The reason we have NMethodIterator and
> CompiledMethodIterator is that we sometimes want to make sure we only
> iterate over nmethods. Hope that works.
> 
> Here is an updated webrev with the other changes you commented on:
> 
> http://cr.openjdk.java.net/~rbackman/8152664.4/
> 
> Thanks
> /R
> 
> On 04/19, Volker Simonis 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 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
>>>