RFR: Update thread local bad mask during root scanning

[Per Liden]

Thanks for reviewing. I'll push this for now, but let's keep this in 
mind. This issues will be highlighted and revisited if/when we start to 
do even more stuff in the thread closure.

cheers,
Per


On 02/22/2018 11:26 AM, Stefan Karlsson wrote:
> 
> On 2018-02-22 08:37, Per Liden wrote:
>> Patch to update the thread local address bad mask during root scanning 
>> while we're anyway walking the thread list (in parallel), instead of 
>> doing a separate single threaded walk over that list.
>>
>> This patch also removes some platform dependent code, and updates the 
>> thread local bad mask on all platforms, even if it's strictly not 
>> needed on e.g. Solaris/SPARC. However, it doesn't hurt performance 
>> wise, and keeps the model nicely homogeneous across platforms.
>>
>> Webrev: 
>> http://cr.openjdk.java.net/~pliden/zgc/update_thread_local_address_bad_mask/webrev.0 
> 
> 
> I'm OK with this patch, but Per and I discussed this briefly:
> 
> This patch has an optimization to reduce the number of times we iterate 
> over the threads, by combining the thread mask setting and the root 
> scanning. This is good for performance, but it breaks the abstraction of 
> ZRootsIterator by letting it perform a task it's not intended to do.
> 
> The two tasks (root scanning and mask updating) are hard-coded in 
> ZRootsIterator::do_threads and ZRootsIteratorThreadClosure, but we could 
> at least decouple this, by extracting the mask updating in a closure 
> that gets injected into ZRootsIterator. If more things were to be added 
> to be done during the threads iteration, only the injected closure would 
> have to be changed, but not the ZRootsIterator.
> 
> The decoupling has the unfortunate property that we would have to 
> propagate the closure through a couple of layers, so for now we are 
> leaving the code as proposed in the patch.
> 
> The layering would be something like this:
> 
> void ZHeap::mark_start() {
> ...
>    ZAddressMasks::flip_to_marked(); // Original mask flipping location
> ...
>    _mark.start(flip_to_marked_thread_closure()); // New closure
> }
> 
> void ZMark::start(ThreadClosure* flipped_to_marked_cl) {
> ...
>    ZMarkRootsTask task(this, flipped_to_marked_cl);
>    _workers->run_parallel(&task);
> }
> 
> ZMarkRootsTask(ZMark* mark, ThreadClosure* flipped_to_marked_cl) :
>      ZTask("ZMarkRootsTask"),
>      _mark(mark),
>      _roots(flipped_to_marked_cl) {}
> 
> ZRootsIterator::ZRootsIterator(ThreadClosure* extra_thread_cl) :
> 
> and then ZRootsIterator::do_threads could apply the closure.
> 
> StefanK
> 
>>
>>
>> cheers,
>> Per
>>