RFR: Refactoring ShControlThread::run_service() method
Aleksey Shipilev
shade at redhat.com
Thu Oct 25 15:25:00 UTC 2018
On 10/25/2018 04:55 PM, Zhengyu Gu wrote:
> Breaking up lengthy ShControlThread::run_service() method, moving setup, prepare and execute GC into
> separate methods.
>
> The patch should not have functional differences.
>
> Webrev: http://cr.openjdk.java.net/~zgu/shenandoah/cleanup_ctrl_thr/webrev.00/index.html
Massaged patch, apply on top:
http://cr.openjdk.java.net/~shade/shenandoah/sh-control-thread-1.patch
I am still not sure this is needed or sensible to do. The current code has particular symmetries
that are broken if you hide parts of the bulk method in submethods. For example,
set_should_clear_all_soft_refs handling should be on the same level, but it is not now. It is not
evident it would be unset on all paths. Another example: there is (pre|post)_gc_work, but some of
that work is done _outside_ the method because it needs alloc_failure_pending/explicit_gc_requested
flags.
Anyhow, what I want to see in proper review is the *entire* thing sliced by changeset: first
refactoring, then the fix. Note you can RFR multiple changesets at once in when doing this. It would
then become evident that: a) refactoring was needed; b) refactoring simplified things for new
feature; c) what are the new things in new feature; d) refactoring was complete, and no other
refactoring touchups were needed to introduce the feature.
-Aleksey
More information about the shenandoah-dev
mailing list