RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

[Doerr, Martin]

Hi Erik,

this seems to be a valid workaround for GCC: I got sizeof(IsUnloadingUnion) = 1
But it's just a GCC hack.

Best regards,
Martin


-----Original Message-----
From: Erik Österlund <erik.osterlund at oracle.com> 
Sent: Montag, 5. November 2018 17:18
To: Doerr, Martin <martin.doerr at sap.com>
Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

Hi Martin,

Oh snap. If I change the IsUnloadingStruct to use 
__attribute__((packed)), it becomes one byte as I expected. Does that 
work for you?

Thanks,
/Erik

On 2018-11-05 17:11, Doerr, Martin wrote:
> Hi Erik,
>
> gdb shows sizeof(IsUnloadingUnion) = 4 on linux x86_64, PPC64 BE, PPC64 LE and s390.
> I havent't used a debugger on AIX, but I believe it's the same there.
>
> Unfortunately, C++ doesn't guarantee a certain size. So it's incorrect for BE platforms in general.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Erik Österlund <erik.osterlund at oracle.com>
> Sent: Montag, 5. November 2018 16:53
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR: 8209189: Make CompiledMethod::do_unloading more concurrent
>
> Hi Martin,
>
> Interesting. The issue seems to be that I assumed that this struct:
>
> struct IsUnloadingStruct {
>     unsigned int _is_unloading:1;
>     unsigned int _unloading_cycle:2;
> };
>
> ...would be one byte large, as I explicitly asked for just 3 bits for
> both bit fields combined. If that really has the size of 4, then that is
> problematic indeed. But I am not sure how it could be 4 bytes, when I
> explicitly sized the number of bits I want in the integers so that I
> would only need 3 bits, with the closest type holding that being a byte.
>
> Is this happening with the xlc compiler only? Do I need some packing
> attribute to the struct to make it pack the field into a byte? Does it
> help if the integers are declared as uint8_t instead?
>
> I have not observed any problems on SPARC.
>
> Thanks,
> /Erik
>
> On 2018-11-05 16:41, Doerr, Martin wrote:
>> Hi Erik,
>>
>> we observe crashes on Big Endian PPC64 machines since this change was pushed.
>>
>> Taking a look at the IsUnloadingUnion which looks incorrect to me.
>> Note that sizeof(IsUnloadingUnion) = sizeof(IsUnloadingStruct) = 4 on PPC64.
>> On Big Endian, the 8 bit _value and the _inflated bits are disjoint.
>>
>> I haven't checked SPARC, yet.
>>
>> Can you take a look, please?
>>
>> Thanks and best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of Erik Osterlund
>> Sent: Montag, 5. November 2018 07:20
>> To: David Holmes <david.holmes at oracle.com>
>> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: RFR: 8209189: Make CompiledMethod::do_unloading more concurrent
>>
>> Hi David,
>>
>> Yes that lock is now unused. Sorry I forgot to delete it!
>>
>> /Erik
>>
>>> On 4 Nov 2018, at 22:38, David Holmes <david.holmes at oracle.com> wrote:
>>>
>>> Hi Erik,
>>>
>>> Just a follow-up on this change. It seems that the CodeCacheUnloadingTask::_lock is now unused - is that correct? (You (accidentally?) made it non-private as part of this change.)
>>>
>>> I was moving that lock as part of the static initalization cleanup, but will just delete it if now unused.
>>>
>>> Thanks,
>>> David
>>>
>>>> On 20/10/2018 12:22 AM, Erik Österlund wrote:
>>>> Hi,
>>>> Today the nmethods are unloaded either serially or in parallel due to GC (and e.g. class unloading). With ZGC concurrent class unloading, a concurrent fashion is required. Rather than inventing yet a third way of unloading nmethods due to class unloading, it would be ideal if there could be one unified way of doing it that makes everyone happy.
>>>> To solve this problem in a more general way, a new member function on CompiledMethod is added: is_unloading(). It returns whether a CompiledMethod has broken oops. In order to not have to iterate over all the oops every time this question is asked, it caches the result, so it needs to be computed only once per "epoch". Every time a CodeCache unloading is triggered by the GC, a new epoch is started, which forces re-computation of whether the CompiledMethod is_unloading() the first time it is called.
>>>> So by having is_unloading() be lazily evaluated, it is now possible to build a do_unloading() method on nmethod that simply makes the nmethod unloaded if it is_unloading(), and otherwise cleans the various caches. Cleaning e.g. the inline caches of said nmethod, uses the same is_unloading() method on its targets to figure out if the IC cache should be cleaned or not. Again, the epoched caching mechanism makes sure we don't recompute this value.
>>>> The new do_unloading() method may be called in both serial, parallel and concurrent contexts. So I removed the parallel variation of this that we had before, that unnecessarily postponed the unloading due to not having computed whether the nmethod should be unloaded or not. Since that is now computed on-demand lazily, there is no longer a need for postponing this, nor to have two phases for parallel nmethod unloading.
>>>> While there is more work involved in making concurrent nmethod unloading work, this is a good cleanup towards that goal.
>>>> Bug ID:
>>>> https://bugs.openjdk.java.net/browse/JDK-8209189
>>>> Webrev:
>>>> cr.openjdk.java.net/~eosterlund/8209189/webrev.00/
>>>> Thanks,
>>>> /Erik