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