RFR: 8311883: [Genshen] Adaptive tenuring threshold
Y. Srinivas Ramakrishna
ysr at openjdk.org
Tue Jul 11 18:02:31 UTC 2023
On Wed, 28 Jun 2023 03:00:51 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:
> Consider using the census information collected at marking in the current cycle (and perhaps at evacuation in the previous cycle?) to inform the generation sizing/budgeting for the evacuation cycle to follow. Talk with Kelvin to discuss a suitable API and place to hook this into.
Tracked in a separate PR/JBS.
> src/hotspot/share/gc/shenandoah/shenandoahAgeCensus.cpp line 120:
>
>> 118: _global_age_table[_epoch]->clear();
>> 119: CENSUS_NOISE(_global_noise[_epoch].clear();)
>> 120: if (!GenShenCensusAtEvac) {
>
> In the case of `GenShenCensusAtEvac` this work is done by the calling method explicitly via `ingest` and `compute_...`
>
> This should probably be done in a separate method/API to keep the API/spec of this method clean.
Done
> src/hotspot/share/gc/shenandoah/shenandoahAgeCensus.cpp line 174:
>
>> 172: // that all higher ages have a mortality rate that is below a
>> 173: // pre-specified threshold. We consider this to be the adaptive
>> 174: // tenuring age to be used for the next cohort.
>
> ... for the next _cycle_.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahAgeCensus.cpp line 196:
>
>> 194: // cohorts are considered eligible for tenuring when all older
>> 195: // cohorts are.
>> 196: if (i > 1 && (prev_pop < GenShenTenuringCohortPopulationThreshold ||
>
> Consider getting a more accurate population for cohort 0 (by adding the population in each region above TAMS), so that we may consider the case of `i = 1` also above (i.e. `i >=1`). This would potentially allo tenuring threshold to go as low as 1 in some cases. This choice will need to be carefully considered.
Done. Default ceiling is set by min age which defaults to 1.
> src/hotspot/share/gc/shenandoah/shenandoahAgeCensus.cpp line 249:
>
>> 247: log_info(gc, age)(UINTX_FORMAT "\t\t " SIZE_FORMAT "\t\t " SIZE_FORMAT "\t\t %.2f " ,
>> 248: (uintx)i, prev_pop, cur_pop, mr);
>> 249: }
>
> vertical alignment via tabbing. Specify units for volume.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 442:
>
>> 440: LogStream ls(lt);
>> 441: heap->phase_timings()->print_cycle_on(&ls);
>> 442: evac_tracker->print_evacuations_on(&ls, &evac_stats.workers,
>
> Does this need to be an instance method?
It's a bit awkward to keep it static, and the cost of calling the instance method no worse. (We actually just make use of a single instance field `_generational` in the method. We could make that field static and leave the method static, but I am not sure it's worth doing.
> src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 50:
>
>> 48: _bytes_completed += bytes;
>> 49: if (_generational && GenShenCensusAtEvac && young_gen) {
>> 50: assert(age > 0, "Error");
>
> Check that age is recorded _after_ incrementing at evacuation.
Yes, it is on the path where we evacuate. It isn't for the case where it's on the marking path. (This is of course the evacuation path here.)
> src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 62:
>
>> 60: if (_generational && GenShenCensusAtEvac) {
>> 61: // Discard const
>> 62: _age_table.merge(((ShenandoahEvacuationStats*)other)->age_table());
>
> Is the cast necessary?
Avoided the cast by moving age table out of the structure (which makes sense anyway since it's not used in some cases).
> src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 69:
>
>> 67: _evacuations_completed = _evacuations_attempted = 0;
>> 68: _bytes_completed = _bytes_attempted = 0;
>> 69: if (_generational && GenShenCensusAtEvac) {
>
> Avoid GenShen: use Shenandoah.
Done for this and other flags beginning "GenShen*" -> "ShenandoahGenerational*".
Please feel free to suggest better (and/or shorter) names.
> src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 112:
>
>> 110: } else {
>> 111: assert(r->is_old(), "Sanity");
>> 112: old_region_ages.add(r->age(), r->get_live_data_words());
>
> We currently don't do anything with ages of old regions as far as I can tell. Perhaps the idea is to use it in some way in the future?
Leaving as is for now for informational purposes; we can deal with this suitably in the future if/when we use it in some way. We might also decide to elide it until such time.
> src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 121:
>
>> 119: st->cr();
>> 120: st->print("Old regions: ");
>> 121: old_region_ages.print_on(st, tenuring_threshold);
>
> `tenuring_threshold` in this context doesn't make any sense for old regions.
Changed to just use the max tenuring threshold here, although that doesn't really make sense here either. We can rethink this in the future, or elide that part. Leaving as is (using max tenuring threshold) for the moment.
> src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 472:
>
>> 470: _worker_id(worker_id) {
>> 471: if (_heap->mode()->is_generational()) {
>> 472: _tenuring_threshold = _heap->age_census()->tenuring_threshold();
>
> Is this updated in the most recent stop-world marking, or is it using the one from the most recent concurrent collection? Could an interrupted concurrent marking cause a bogus census to be used here?
>
> Partial censuses should be expunged.
It is true that cancellations could lead to bogus partial censuses being used. I'll locate the right place in the cancellation path(s) where partial censuses will need to be expunged. Essentially, these would be the points where concurrent marking is canceled (when censuses are conducted during the marking phase) and where concurrent evacuation is canceled (when censuses are conducted during the evacuation phase). All other cancellations are fine because the census data will be valid despite cancellation in those cases.
> src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 482:
>
>> 480: if (!GenShenCensusAtEvac) {
>> 481: // Age table updates
>> 482: heap->update_epoch();
>
> A different name for the method to avoid confusion.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 491:
>
>> 489: AgeTable* get_age_table();
>> 490: void update_epoch();
>> 491: void reset_epoch();
>
> `epoch` overload/confusion with prior use. Consider if this is really needed; if so, find better names to avoid confusion with other uses of the term `epoch` or methods thereof.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 484:
>
>> 482: // If copying to the old generation, we don't care about recording
>> 483: // object age in the census stats.
>> 484: _evac_tracker->end_evacuation(thread, size * HeapWordSize, young_gen, young_gen ? ShenandoahHeap::get_object_age(copy_val) : 0);
>
> Revisit this to see why we record age 0 for the case of old gen objects.
To answer the question, "why 0?" : it was because I didn't want to extract the age for the old_gen case, because it isn't used in that case (the census doesn't record old gen object ages). I'll see about cleaning this up a bit.
> src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 549:
>
>> 547: }
>> 548:
>> 549: uint ShenandoahHeap::get_object_age_concurrent(oop obj) {
>
> Pla ce documentation comments distinguishing between the two methods here too.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp line 468:
>
>> 466: uint youth() {
>> 467: return CENSUS_NOISE(_youth)
>> 468: NO_CENSUS_NOISE(0);
>
> May nit need in product build.
Indeed. Fixed.
> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp line 479:
>
>> 477: CENSUS_NOISE(_youth++;)
>> 478: }
>> 479: }
>
> Unused: remove.
Removed.
> src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 60:
>
>> 58: }
>> 59:
>> 60: template <class T, ShenandoahGenerationType GENERATION, StringDedupMode STRING_DEDUP>
>
> Consider extracting the GENERATION type from T (closure's type).
This might be possible via the suggestion/comment further down at line 102. However, this might require a bit more refactoring of other code, which I'd prefer to do not here in this ticket, but separately.
I'll defer that to a separate PR for which I'll open a JBS in anticipation and link it here.
> src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 102:
>
>> 100: // from final- to strong mark.
>> 101: if (task->count_liveness()) {
>> 102: count_liveness<GENERATION>(live_data, obj, worker_id);
>
> Experiment with pulling count_liveness into the closure `cl` which already knows the generation type.
See comment at line 60 further above. This will be addressed separately as a refactor to follow to avoid complicating this PR with more changes.
> src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 118:
>
>> 116:
>> 117: // Age census for young objects
>> 118: if (GENERATION == YOUNG && !GenShenCensusAtEvac) {
>
> Fold the two together.
Done.
> src/hotspot/share/gc/shenandoah/shenandoahPacer.hpp line 30:
>
>> 28: #include "gc/shenandoah/shenandoahNumberSeq.hpp"
>> 29: #include "gc/shenandoah/shenandoahPadding.hpp"
>> 30: #include "gc/shenandoah/shenandoahSharedVariables.hpp"
>
> Check this again.
Needed for `ShenandoahSharedFlag` (at least). A previous include somewhere else satisfied all previous uses which unravelled when some of the files (notably shenandoah thread local data's .cpp file was moved to its own file).
Leaving as is.
> src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 38:
>
>> 36: constraint) \
>> 37: \
>> 38: product(bool, GenShenCensusAtEvac, true, EXPERIMENTAL, \
>
> GenShen -> ShenandoahGenerational, perhaps.
Done for all these "GenShen" flags.
> src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 39:
>
>> 37: \
>> 38: product(bool, GenShenCensusAtEvac, true, EXPERIMENTAL, \
>> 39: "Object age census at evacuation, not mearking") \
>
> "marking"
Done.
> src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 47:
>
>> 45: "Floor for adaptive tenuring threshold") \
>> 46: \
>> 47: product(uintx, GenShenMaxTenuringThreshold, 16, EXPERIMENTAL, \
>
> Why not 15?
We'll need to discuss this. Allowing this to be 16 (i.e. 1 greater than max_age) allows for a potential "never tenure" behavior. Otherwise, age 15 objects would always become eligible for tenuring. I think allowing it to be above 15 (i.e. allowing 16, and allowing objects to stay in young till and including age 15) would seem to make sense.
I'll set the default to 15 (thus never allowing objects that , but allow users to set it to 16, simulating "never tenure", if both min and max are set to 16.
> src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 50:
>
>> 48: "Ceiling for adaptive tenuring threshold") \
>> 49: \
>> 50: product(double, GenShenTenuringMortalityRateThreshold, 0.3, EXPERIMENTAL, \
>
> Experiment with different values here (0.1 may be more appropriate perhaps?).
>
> Add guidance about which way (lower) to move this parm to keep objects in young gen longer.
Done; changed down to 0.1, added documentation guidance. Experiments pending to establish what might work well most of the time.
-------------
PR Comment: https://git.openjdk.org/shenandoah/pull/289#issuecomment-1619350122
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247185934
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247192057
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1258995056
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251227689
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247245894
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247258164
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247276576
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248232985
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248228695
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248227506
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1259042906
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247032066
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1247021917
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248287192
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251205809
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248358730
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248358697
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251362713
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251362652
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251213365
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251357086
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248324863
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1248340538
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1251387356
PR Review Comment: https://git.openjdk.org/shenandoah/pull/289#discussion_r1258985040
More information about the shenandoah-dev
mailing list