RFR: 8210498: nmethod entry barriers

[Roman Kennke]

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