RFR: Change affiliation representation

Y. Srinivas Ramakrishna ysr at openjdk.org
Mon Nov 14 23:26:32 UTC 2022


On Mon, 14 Nov 2022 21:14:53 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> With generational mode of Shenandoah, each region is associated with OLD, YOUNG, or FREE.  During certain marking and update-refs activities, the region affiliation is frequently consulted.  In the original implementation, the region affiliation was stored in a field of the ShenandoahHeapRegion object.  In this code, we maintain a separate array, indexed by region number, to represent the affiliation of each region.  This saves one level of indirection and improves cache locality for looking up the affiliation of each region.
> 
> Measurements show significant improvement in throughput.  One workload that was configured to perform back-to-back old-gen collections was able to increase the frequency of old-gen collections by almost 5 fold.  With a 20-minute Extremem workload using a 48G heap, 20G old-gen, the P95 latency improvement was 0.54% (2.395 ms) and the P99.999 latency improvement was 58.21% (29.195 ms) in comparison to the implementation before this patch.

The change looks good, modulo William's comment. (I also wonder if it might make sense to have a GenerationalShenandoahHeap derived from ShenandoahHeap that might make some of this kind of separation a bit cleaner. But that would be a bigger change for the future if one thought it was worthwhile to do a clean separation of the code for the two collectors.)

A high level question, unrelated to the changes here. 

I imagine that we never consult a region's affiliation while it may be concurrently subject to update? (or when that happens, that any resulting race is benign)? Would it be worth adding a brief comment to that effect somewhere? (may be where the _affiliation array is declared.)

I am guessing the affiliation state graph looks like:

free -> {young, old}
young -> {old, free}
old -> free

Would this be worth adding as a comment may be where set_affiliation() is implemented?

None of these need to be done right away, but thought I'd leave these thoughts in the review anyway.

-------------

Marked as reviewed by ysr (Author).

PR: https://git.openjdk.org/shenandoah/pull/170


More information about the shenandoah-dev mailing list