[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR: 8234654: ZGC: Only disarm NMethods when marking/relocating code roots
From:       Erik_Österlund <erik.osterlund () oracle ! com>
Date:       2019-12-10 8:20:50
Message-ID: A9FBCB03-9E27-49F9-B58B-225574BF1592 () oracle ! com
[Download RAW message or body]

Hi Stefan,

Looks... even better!

Thanks,
/Erik

> On 9 Dec 2019, at 22:29, Stefan Karlsson <stefan.karlsson@oracle.com> wrote:
> 
> Reworked the patch a bit, because I wanted to move _disarm_nmethod out from the \
> iterator and into the closures: 
> https://cr.openjdk.java.net/~stefank/8234654/webrev.01.delta/
> https://cr.openjdk.java.net/~stefank/8234654/webrev.01/
> 
> - Ripped out _disarm_nmethod from the ZRootsIterator.
> 
> - Added a should_disarm_nmethod() virtual function to the closure. The mark and \
> relocation closures return true. Had a version that introduced a \
> ZRootsIterator::do_code_blob analogous to ZRootsIterator::do_thread, but we don't \
> need that flexibility right now, so backed away from that in favor of more straight \
> forward code. 
> - Removed closure indirection in ZRootsIteratorCodeBlobClosure.
> 
> - Changed to use ZNMethod::nmethod_oops_do instead of nm->oops_do and \
> nm->fix_oop_relocations, since it's faster . 
> - Changed to use ZNMethod::disarm(nm) instead of _bs->disarm(nm), since it hides \
> the _bs retrieval. 
> - Added armed state assert.
> 
> This passes tier1-7 of ZGC testing.
> 
> Sat down with Per for the final version of this. Either we fold this patch into his \
> change, or I'll add this as a patch afterwards. 
> Thanks,
> StefanK
> 
> 
> > On 2019-12-09 10:17, Per Liden wrote:
> > Here's an updated webrev, which removes the "assert(is_armed, ...)" \
> > BarrierSetNMethod::disarm(), as it's a bit too strict. Also, added the "if \
> > (ZNMethod::is_armed(nm))" to the ZNMethodUnlinkClosure, so avoid walking to oops \
> > if it's already disarmed. 
> > http://cr.openjdk.java.net/~pliden/8234654/webrev.1
> > 
> > /Per
> > 
> > > On 11/22/19 3:38 PM, Per Liden wrote:
> > > Thanks Erik!
> > > 
> > > /Per
> > > 
> > > On 11/22/19 3:35 PM, erik.osterlund@oracle.com wrote:
> > > > Hi Per,
> > > > 
> > > > Looks good.
> > > > 
> > > > Thanks,
> > > > /Erik
> > > > 
> > > > On 11/22/19 3:03 PM, Per Liden wrote:
> > > > > ZRootIterator will currently always try to disarm on-stack NMethods. \
> > > > > Strictly speaking, we should only do this when marking/relocating code \
> > > > > roots, not when e.g. iterating the heap. 
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8234654
> > > > > Webrev: http://cr.openjdk.java.net/~pliden/8234654/webrev.0
> > > > > 
> > > > > /Per
> > > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]