RFR: 8213755: Let nmethods be is_unloading() outside of safepoints

[Erik Österlund]

Hi Coleen,

Thanks for the review.

On 2018-11-19 23:42, coleen.phillimore at oracle.com wrote:
>
>
> On 11/19/18 7:31 AM, Erik Österlund wrote:
>> Hi Coleen,
>>
>> Thanks for having a look at this.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00_01/
>>
>> On 2018-11-17 00:21, coleen.phillimore at oracle.com wrote:
>>>
>>> How about instead of two bool arguments that are hard to read and 
>>> error prone and need comments, like:
>>>
>>> 76 CompiledMethodIterator iter(true /* only_alive */, false /* 
>>> only_not_unloading */);
>>>
>>>
>>> enum FilterAlive { all_blobs, only_alive };
>>> enum FilterUnloading { all_blobs, only_not_unloading };
>>>
>>> Then it can be something like:
>>>
>>>    CompiledMethodIterator iter(only_alive, all_blobs);
>>>
>>> Don't know if you can repeate all_blobs like that.
>>
>> You can't really have all_blobs in both. I tried out a variant with a 
>> single enum though, hope you like it.
>> The observation was that we only need 3 different variations: 
>> all_blobs, only_alive or only_alive_and_not_unloading. So I made an 
>> enum out of that.
>> In fact, these three modes are the only ones I support right now, so 
>> they are also the only valid options, which made the single enum look 
>> even more reasonable.
>
> I do like it!
>>
>>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/src/hotspot/share/code/codeCache.cpp.frames.html 
>>>
>>>
>>> I'm not sure if metadata_do() should filter out unloading. It's 
>>> looking for "old" methods to stop from deallocating memory for them 
>>> (but it's in a safepoint).  If you're unloading, the IC caches are 
>>> already cleaned out?
>>
>> IC caches to unloading nmethods are not necessarily cleaned out. The 
>> concurrent GC threads will clean the IC caches concurrently, but a 
>> JavaThread can call the IC cache before the GC has gotten around to 
>> cleaning the IC cache. But any such call into an is_unloading() 
>> nmethod will be trapped by nmethod entry barriers, which will call 
>> the IC miss handler, which sorts things out lazily. So you can view 
>> this as the IC caches to is_unloading() nmethods being logically 
>> cleared, but not necessarily physically cleared yet. The effect is 
>> the same - the is_unloading() nmethods may not be entered from 
>> anywhere, including IC caches.
>>
>> So if you were to not filter is_unloading(), you would get a whole 
>> bunch of nmethods that are not really reachable by the application, 
>> i.e. they are not on any stacks, and may not be entered by the 
>> application. Therefore, I think it's perfectly fine to filter them 
>> out here, unless I missed something. And this makes the behaviour 
>> closer to as-if the nmethods were indeed unloaded in the safepoint.
>
> Ok.  The thing I'm worried about is if we deallocate memory for a 
> Method* in the is_unloading methods, and some other cleaning or 
> sweeping thread tries to access that memory.  This is used by 
> MetadataOnStackMark during safepoints to see what's safe to deallocate 
> from redefinition.

I understand. I do think this is safe though, and it only walked 
is_alive() compiled methods looking for metadata before, e.g. not 
unloaded or zombie methods. Similarly, nobody should be poking around at 
the metadata of unloading methods. Should being the key word there I 
guess...

>
>>
>>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/src/hotspot/share/code/nmethod.cpp.frames.html 
>>>
>>>
>>> 1108 // The release is only needed for compile-time ordering, as 
>>> accesses
>>> 1109 // into the nmethod after the store is not safe, due to the 
>>> sweeper
>>> 1110 // being allowed to free it then, in the case of concurrent 
>>> nmethod
>>> 1111 // unloading. Therefore, there is no need for acquire on the 
>>> loader side.
>>> 1112 OrderAccess::release_store(&_state, (signed char)unloaded);
>>>
>>> I tried to make sense out of this first sentence, but couldn't 
>>> really.  After the store to unloaded, can the sweeper free the 
>>> nmethod?  maybe remove "then, " and it would make more sense?
>>
>> Right, after the store is observed, the sweeper thread may 
>> concurrently observe the nmethod to be unloaded, and then it may 
>> immediately flip it over to zombie. And I don't want any concurrent 
>> make_unloaded code in that nmethod to still be racing after the 
>> sweeper turns it into zombie; a very unnecessary race to deal with.
>>
>> I updated the comment - hopefully it reads better now.
>>
>
> How about removing a comma changing it to accesses are not safe, or 
> access is not safe.
> + // The release is only needed for compile-time ordering, as accesses
> + // into the nmethod after the store are not safe, due to the sweeper
> + // being allowed to free it when the store is observed during
> + // concurrent nmethod unloading. Therefore, there is no need for
> + // acquire on the loader side.

Comma will be removed before pushing.

> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.01/src/hotspot/share/code/codeCache.hpp.udiff.html
>
> Why is this copy constructor defined?  Isn't this what the default 
> copy constructor would do?

Yeah it is. I like explicitly typing out what constructors should do. 
But I can remove it if you prefer that.

> It looks almost perfect to me :)

Awesome. Thanks for reviewing this.

Thanks,
/Erik

> Coleen
>
>> Thanks,
>> /Erik
>>
>>> Thanks,
>>> Coleen
>>>
>>> On 11/12/18 5:46 PM, Erik Österlund wrote:
>>>> ..put in bug number in subject to make mail filters happy.
>>>>
>>>> /Erik
>>>>
>>>> On 2018-11-12 23:44, Erik Österlund wrote:
>>>>> Hi,
>>>>>
>>>>> All current GCs perform code cache unloading in safepoints. 
>>>>> Therefore, nmethods that are alive but is_unloading() are never 
>>>>> observed outside of safepoints. With concurrent class unloading, 
>>>>> nmethods that are alive but is_unloading() will become observable 
>>>>> outside of safepoints. This must be handled appropriately.
>>>>>
>>>>> In this patch I changed the nmethod/compiled method iterators to 
>>>>> accept parameters describing whether they should filter out not 
>>>>> is_alive() or is_unloading() methods. Since there is no obvious 
>>>>> default (all combinations are used depending on call site), you 
>>>>> have to explicitly set what iteration mode you want.
>>>>>
>>>>> Other than that, I make sure that the sweeper stays away from 
>>>>> is_unloading() nmethods that are not yet is_unloaded(). To make 
>>>>> the interactions between the sweeper and concurrent GC threads 
>>>>> safe, I had to move down the store that sets the state to 
>>>>> unloaded, and use a release_store there, to make sure no accesses 
>>>>> float below it at compile-time. Once that store is observed, 
>>>>> nmethods may be deleted.
>>>>>
>>>>> In the IC miss handler, I also need to lazily clean stale IC 
>>>>> caches due to calling is_unloading nmethods using nmethod entry 
>>>>> barriers.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8213755
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>
>>
>