RFR: 8210498: nmethod entry barriers

[Per Liden]

On 2018-10-08 10:19, Erik Österlund wrote:
> Hi Per and Roman,
> 
> Thank you for having a look at this.
> I went with Roman's suggestion and made this an explicit opt-out 
> instead. But I let ModRef pass in NULL, as none of the barrier sets in 
> that category currently use nmethod entry barriers. When they start to 
> do so, that can be easily changed.

Sounds good.

> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8210498/webrev.00_01/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8210498/webrev.01/

Looks good!

cheers,
Per

> 
> Thanks,
> /Erik
> 
> On 2018-10-08 08:48, Roman Kennke wrote:
>> I agree. I wouldn't mind to keep it simple and pass NULL though. I'm not
>> a big fan of default arguments anyway.
>>
>> Roman
>>
>>> On 2018-10-05 17:07, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> In order to implement concurrent class unloading for ZGC, a mechanism
>>>> will be required to arm nmethods in a safepoint such that the first
>>>> entry into an nmethod after the pause triggers a barrier before
>>>> continuing.
>>>>
>>>> The barrier will be used for:
>>>>     * Patching immediate oops, and keeping phantomly reachable oops
>>>> alive through the nmethod entry
>>>>     * Catching calls from stale IC caches (due to class unloading) into
>>>> nmethods that are being unloaded (containing broken oops and
>>>> metadata), and lazily clean the IC cache and re-resolve the call.
>>>>
>>>> This implementation is for x86_64 and is based on the prototyping work
>>>> of Rickard Bäckman. So big thanks to Rickard, who will co-author of
>>>> this patch.
>>>> The way it works is that there is a cmp #offset(r15_thread),
>>>> simm32_epoch); je #continuation; inserted into the verified entry of
>>>> nmethods that conditionally calls a stub trampolining into the VM.
>>>> When the simm32_epoch is not equal to some TLS-local field, then a
>>>> slow path is taken to run some barrier inside of the VM.
>>>> By changing this TLS-local field in the safepoint, all verified
>>>> entries will take the slow path into the VM when run, where the
>>>> barrier is eventually disarmed by patching the simm32_epoch value of
>>>> the instruction stream to the current globally correct value.
>>>>
>>>> In terms of abstractions, to utilize this mechanism, you need to
>>>> create your BarierSet with an BarrierSetNMethod helper object, just
>>>> like the BarrierSetAssembler, BarrierSetC1 and BarrierSetC2. The
>>>> BarrierSetNMethod base class handles the barrier itself. It has a
>>>> virtual function, nmethod_entry_barrier, which gets called by the slow
>>>> paths. So each user of this mechanism inherits from BarrierSetNMethod
>>>> and puts its barrier in there.
>>>>
>>>> The OSR nmethods do not have a verified entry. Therefore, the barrier
>>>> is run explicitly in the runtime before calling into OSR nmethods to
>>>> achieve the same effect of trapping new calls into nmethods.
>>>>
>>>> The nmethod entry barrier returns true/false depending on whether the
>>>> called nmethod may be entered or not. This allows lazily cleaning
>>>> nmethod IC caches during unloading, which invokes the same helper
>>>> method that is patched into the verified entry when made not entrant.
>>>>
>>>> Eventually, I would like to also use this mechanism to remove the
>>>> nmethod hotness counter maintenance we today perform in safepoint
>>>> cleanup instead. These nmethod entry barriers can provide better
>>>> heuristics for whether nmethods are used or not. But that would
>>>> require buy-in from maintainers of other platforms. So let's take one
>>>> step at a time.
>>>>
>>>> For the curious reader thinking that this might be an expensive
>>>> operation, it is not. This check in the nmethod verified entry is very
>>>> predictable and cheap. And because of use of inlining, the checks
>>>> become more infrequent. In my evaluation, it has gone completely under
>>>> the radar in benchmarking (running this with my current version of
>>>> concurrent class unloading in ZGC).
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8210498/webrev.00/
>>> I reviewed this before Erik sent it out, so my comments have already
>>> been addressed. Just found one additional thing, the BarrierSet
>>> constructor now looks like this:
>>>
>>> 96   BarrierSet(BarrierSetAssembler* barrier_set_assembler,
>>> 97              BarrierSetC1* barrier_set_c1,
>>> 98              BarrierSetC2* barrier_set_c2,
>>> 99              const FakeRtti& fake_rtti,
>>> 100             BarrierSetNMethod* barrier_set_nmethod = NULL) :
>>> 101     _fake_rtti(fake_rtti),
>>> 102     _barrier_set_assembler(barrier_set_assembler),
>>> 103     _barrier_set_c1(barrier_set_c1),
>>> 104     _barrier_set_c2(barrier_set_c2),
>>> 105     _barrier_set_nmethod(barrier_set_nmethod) {}
>>>
>>> I find it odd that the fake_rtti argument comes in the middle of the
>>> barrier_set_* arguments. I can see that you don't want GC barrier sets
>>> to have to supply NULL if they don't have a nmethod barrier set, but
>>> maybe we can have two constructors instead, like:
>>>
>>> BarrierSet(BarrierSetAssembler* barrier_set_assembler,
>>>             BarrierSetC1* barrier_set_c1,
>>>             BarrierSetC2* barrier_set_c2,
>>>             const FakeRtti& fake_rtti) :
>>>      _fake_rtti(fake_rtti),
>>>      _barrier_set_assembler(barrier_set_assembler),
>>>      _barrier_set_c1(barrier_set_c1),
>>>      _barrier_set_c2(barrier_set_c2),
>>>      _barrier_set_nmethod(NULL) {}
>>>
>>> BarrierSet(BarrierSetAssembler* barrier_set_assembler,
>>>             BarrierSetC1* barrier_set_c1,
>>>             BarrierSetC2* barrier_set_c2,
>>>             BarrierSetNMethod* barrier_set_nmethod,
>>>             const FakeRtti& fake_rtti,) :
>>>      _fake_rtti(fake_rtti),
>>>      _barrier_set_assembler(barrier_set_assembler),
>>>      _barrier_set_c1(barrier_set_c1),
>>>      _barrier_set_c2(barrier_set_c2),
>>>      _barrier_set_nmethod(barrier_set_nmethod) {}
>>>
>>> What do you think?
>>>
>>> Other than that, this looks good to me.
>>>
>>> cheers,
>>> Per
>>>
>>>> Bug ID:
>>>> https://bugs.openjdk.java.net/browse/JDK-8210498
>>>>
>>>> Thanks,
>>>> /Erik
>