RFR: 8210498: nmethod entry barriers

[Erik Österlund]

Hi Per,

Thanks for the review!

/Erik

On 2018-10-08 10:34, Per Liden wrote:
>
> 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
>>