RFR: Traveral GC heuristics

[Roman Kennke]

Am 19.01.2018 um 11:24 schrieb Aleksey Shipilev:
> On 01/17/2018 10:58 PM, Roman Kennke wrote:
>> Yes, maybe. But for the start, I did not want it to interfere with existing code if I can avoid
>> it. For this reason, this looks like a copy+paste job from conc-mark and partial for some parts.
> 
> Okay, but please plan to common these things right away. We cannot have two copy-pasted 1000+ LOC
> blocks and hope for the best ;)

Well the plan is to get rid of all the other stuff and make traversal 
the GC to rule them all ;-)


>> Thanks for reviewing and spotting all the issues. I could not really make a diff webrev, because I
>> first had to pull -u your latest work, and this messed up my differential webrev... sorry. Only full
>> webrev now:
>>
>> http://cr.openjdk.java.net/~rkennke/traversal/webrev.03/
> 
> Sorry to be a PITA about this, but the change is quite large, and I think we want to be more
> forward-looking to backports and stability.
> 
> Another sweep through the code:
> 
> *) GCCause::to_string misses the to_string case for _shenandoah_traversal_gc?

Added.

> *) So, wait. SBS::nterpreter_write_barrier_impl caller-saves registers when they do not equal to
> dst. New code in SBS::interpreter_storeval_barrier just does it unconditionally. Is WB too cautious,
> or SVB is too lax about this?

Neither. The WB returns a value into the same register as the input 
value. We don't want to trash this when returning. The enqueing barrier 
is a one-way street.

> *) I think with minimal changes, we can make ShenandoahStoreValEnqueueBarrier exclusive, which will
> make testing much easier (encoding this in TestSelectiveBarriers would be trivial). E.g. say:
> 
>     if (UseShenandoahGC) {
>       if (ShenandoahStoreValWriteBarrier || ShenandoahStoreValEnqueueBarrier) {
>         // perform WB
>       }
>       if (ShenandoahStoreValEnqueueBarrier) {
>         // enqueue
>       }
>       if (ShenandoahStoreValReadBarrier) {
>         // RB
>       }
>     }

Done. Altough it turned out to be not so minimal. Needed to add checks 
all over the place.

> *) Minor nit: please indent second arguments like this:
> 
>       FLAG_SET_DEFAULT(UseShenandoahMatrix,              false);
>       FLAG_SET_DEFAULT(ShenandoahSATBBarrier,            false);
>       FLAG_SET_DEFAULT(ShenandoahConditionalSATBBarrier, false);
>       FLAG_SET_DEFAULT(ShenandoahStoreValReadBarrier,    false);
>       FLAG_SET_DEFAULT(ShenandoahStoreValWriteBarrier,   true);
>       FLAG_SET_DEFAULT(ShenandoahStoreValEnqueueBarrier, true);
>       FLAG_SET_DEFAULT(ShenandoahKeepAliveBarrier,       false);
>       FLAG_SET_DEFAULT(ShenandoahAsmWB,                  true);
>       FLAG_SET_DEFAULT(ShenandoahBarriersForConst,       true);
>       FLAG_SET_DEFAULT(ShenandoahWBWithMemBar,           false);
>       FLAG_SET_DEFAULT(ShenandoahWriteBarrierRB,         false);

Done.

> *) shenandoahOopClosures.hpp, indenting is a bit off here:
> 
>   240       _thread(Thread::current()), _queue(q) {}
> 
> ...
> 
>   273   virtual bool do_metadata() { return true; }

Fixed.

> *) I wonder if we want to pull out ShenandoahWBWithMemBar changes into a separate changeset? This
> looks potentially backportable, and usable outside of Traversal GC.

Already done and pushed.

Also, I have added tests that exercise traversal heuristics just as we 
do for other heuristics. This turned up a number of bugs and 
improvements that I fixed:

- when growing the heap, we must make sure that the TAMS points to end 
for the new regions, otherwise we'd treat them implicitely marked.
- added periodic GC
- folded 'SATB' queue processing into thread stack scanning. The problem 
here is that iterating the threads 2x is cumbersome because of the 
claiming protocol: we need to fire the task/workers 2x: once for the 
SATB queues, once for the thread scanning. I folded it into one pass. 
This required a (trivial) extension in the upstream parallel thread 
scanning/iteration protocol.
- I tripped an assert in SHR::increase_live_data(). I think the reason 
is that we have a race here: a GC thread might not yet see the updated 
SHR::_top but already accounts for the updated live data. I excluded 
conc-traversal from that check. This could probably be fixed by doing 
the proper concurrency membars, but do we care? For assertion code?

- interesting bug: in mark-compact, we first check for 
stuff-in-progress, and turn it off. when checking for 
marking-in-progress first, we turn that off first and also turn off 
SATB. Notice the overlap of MARKING with TRAVERSAL. We then go on to 
check for TRAVERSAL, see that it's also ON, turn it off, which also 
turns off SATB again, and trip an assert because it checks the correct 
SATB active state. Reordering the checks fixes this.
- I had a little index-out-of-bounds in humongous-checking code. 
Trivially fixed by bounds-checking.

- Updated patch to match current head (some conflicts with degen)

Differential:
http://cr.openjdk.java.net/~rkennke/traversal/webrev.04.diff/
Full:
http://cr.openjdk.java.net/~rkennke/traversal/webrev.04/

Testing: hotspot_gc_shenandoah passes

Roman