RFR: Refactor budgeting to make the logic cleaner [v2]
Kelvin Nilsen
kdnilsen at openjdk.org
Wed Jul 13 16:27:49 UTC 2022
On Sat, 2 Jul 2022 16:20:04 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:
>>
>> Isolate the code that reserves evacuation budgets into functions
>
> src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 224:
>
>> 222: }
>> 223:
>> 224: void ShenandoahGeneration::compute_evacuation_budgets(ShenandoahHeap* heap, ShenandoahCollectionSet* collection_set,
>
> The refactoring of the large chunks of code into the smaller methods is definitely a step in the right direction in making the code easier to follow.
>
> Nit: Would it make the code more readable to consolidate the long sections of comments into a block comment at the top of each of the methods and have much shorter comments in the body of the methods?
Thanks for your suggestion. Maybe you can do a subsequent pass (in a future PR) to clean up the structure of comments. I'm sure these can be made better. Sometimes, a "reader" of the comments has the best perspective on how to make the comments more valuable to other readers.
> src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 114:
>
>> 112: void prepare_gc(bool do_old_gc_bootstrap);
>> 113:
>> 114: // Compute evacuation budgets prior to choosing collection set.
>
> Do these methods need to be public? They looked more like helper methods in the way they are currently being used.
>
> Another thing to think about is whether the reference parameters in one signature and regular ones in the other are potential sources of confusion or error in the future. I can see why they are like they are in their current use, but might indicate that perhaps a cleaner refactoring of the methods and the state that it uses or modifies, and where that state should reside.
>
> Nothing to do at the moment, but worth thinking about.
Agree with suggestion to make compute_evacuation_budgets and adjust_evacuation_budgets private. Done.
I welcome further refactoring improvements in the future. Am eager to get this code integrated now (as is) so that we can proceed with other performance improvements.
-------------
PR: https://git.openjdk.org/shenandoah/pull/148
More information about the shenandoah-dev
mailing list