RFR: 8210498: nmethod entry barriers

[Per Liden]

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