RFR: Add generations to freeset [v8]
Y. Srinivas Ramakrishna
ysr at openjdk.org
Thu Apr 20 18:18:37 UTC 2023
On Thu, 20 Apr 2023 02:42:11 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 63:
>>
>>> 61:
>>> 62: inline bool ShenandoahFreeSet::in_mutator_set(size_t idx) const {
>>> 63: bool is_mutator_free = _mutator_free_bitmap.at(idx);
>>
>> Should line 63 go after the assert on the next line 64?
>
> Also, it sounds like, in the general case, the assertion check `idx < _max` should apply also to the `probe_` version above? I agree that the other assertion below at line 66 may not apply there.
Also note that the number of lines of code would be 1/3 if we had a set abstraction, with the same implementation you have now, and encapsulated all of this triplicated code into that abstraction, and instantiated it into the three sets. But that refactoring can occur in the future since this would stretch this review and need changes again.
>> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 537:
>>
>>> 535: remove_from_mutator_set(idx);
>>> 536: assert(!in_collector_set(idx) && !in_old_collector_set(idx), "Region cannot be in multiple free sets");
>>> 537: adjust_mutator_bounds_if_touched(idx);
>>
>> Would it be too expensive if `remove_from_mutator_set()` and `add_to_mutator_set()` were to always adjust the bounds if needed (i.e. we traffick an extremal element into or out of the set). Adjustments at additions are of course much less expensive. Then, the interval end-points would always stay faithful to membership index extrema, establishing a good invariant.
>>
>> Is the concern that the adjustments necessitated when removing elements might be too expensive to do at each removal (addition is no problem).
>
> (Similarly at other `adjust_bounds_` methods and their uses below.)
Once again, the consolidation/ecapsulation of all this into the set abstraction would reduce the triplication of the code, and thrice repeated lines that must be kept in sync for any change, increasing maintenance burden.
-------------
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1172026085
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1172837530
More information about the shenandoah-dev
mailing list