RFR: Add generations to freeset [v5]
Kelvin Nilsen
kdnilsen at openjdk.org
Wed Apr 19 14:43:22 UTC 2023
On Thu, 13 Apr 2023 20:28:10 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:
>> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Remove debug instrumentation
>
> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 953:
>
>> 951: }
>> 952:
>> 953: // Evac reserve: reserve trailing space for evacuations
>
> I'd move the individual region assignment loop above into its own work method, and the remainder of the work below that adjusts the reserves into its own work method, if appropriate? Not sure if such a refactor makes sense or not, but might make the code arguable more maintainable/readable?
I've tidied this code and added/improved comments to explain its behavior. I've moved the first loop into its own work method.
> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 976:
>
>> 974: }
>> 975: }
>> 976: reserve_regions(young_reserve, old_reserve);
>
> So I see now that this does a further iteration (below) over the regions until a specific size criteria is met, albeit iterating over the regions in the opposite order. While I have not understood the high level idea of the two loops, one thing to consider if whether the two iterations, one forward and one backward are needed, or if they could somehow be fused into a single iteration. Just tossing it out there as a question that you might be able to answer immediately, probably in the negative.
>
> A brief couple of lines describing the idea of the two loops above and below would be useful.
I've added comments to explain rationale for the backward loop.
-------------
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1171447413
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1171448373
More information about the shenandoah-dev
mailing list