RFR: Usage tracking cleanup

Y. Srinivas Ramakrishna ysr at openjdk.org
Tue Apr 18 21:54:34 UTC 2023 

On Mon, 17 Apr 2023 22:33:05 GMT, William Kemper <wkemper at openjdk.org> wrote:

> There are many nuances to tracing memory utilization. Shenandoah track's usage, waste by humongous objects, padding for promotion LABs alignment and all this is also tracked by generation, the heap and feeds into the heuristics and the pacer. The code to update all of these values and route them to the right places was spread across the allocation call stack. This change consolidates all of the logic into one method, invoked near the end of the allocation.

I have left a few comments. Despite the improvements that you have made by some of the current refactoring, I am concerned about the organization of the allocation paths and believe that there are opportunities for cleaning these up, and specializing and slimming these allocation paths a bit more.

That can wait for later, but I'd like to take another pass at this once you have had a chance to read the review comments and in case those lead to any changes.

Thanks!

src/hotspot/share/gc/shenandoah/shenandoahAllocRequest.hpp line 68:

> 66:   Type _alloc_type;
> 67:   ShenandoahAffiliation const _affiliation;
> 68: #ifdef ASSERT

Nit: Not really your change, but for each of these fields, it'd be great if a 1-line documentation comment were included.

src/hotspot/share/gc/shenandoah/shenandoahAllocRequest.hpp line 118:

> 116:   }
> 117: 
> 118:   inline size_t actual_size() const {

Thank you so much for `const`ing these and others below!

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 249:

> 247: }
> 248: 
> 249: size_t get_usable_free_words(size_t free) {

Please write a block comment:

// This work method takes an argument corresponding to the number of bytes
// free in a region, and returns the largest amount in heapwords that can be allocated
// such that both of the following conditions are satisfied:
//
// 1. it is a multiple of card size
// 2. any remaining shard may be filled with a filler object
//
// The idea is that the allocation starts and ends at card boundaries. Because
// a region ('s end) is card-aligned, the remainder shard that must be filled is
// at the start of the free space.
//
// This is merely a helper method to use for the purpose of such a calculation.


I don't like the scale/unit translation that this method does. You already hint at that via naming the method `get_usable_free_words`. Repeating this in the documentation comment above helps as well. Finally, we can ensure that
the user sees this up front by calling the argument "free_bytes" to further underline this point.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 253:

> 251:   //      free is 514
> 252:   //      usable_free is 512, which is decreased to 0
> 253:   size_t usable_free = (free / CardTable::card_size()) << CardTable::card_shift();

assert(usable_free <= free, "Sanity check");

This is to aid the reader in understanding the flow of logic in the code.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 255:

> 253:   size_t usable_free = (free / CardTable::card_size()) << CardTable::card_shift();
> 254:   if ((free != usable_free) && (free - usable_free < ShenandoahHeap::min_fill_size() * HeapWordSize)) {
> 255:     // We'll have to add another card's memory to the padding

Let's modify the documentation comment:

// After aligning to card multiples, the remainder would be smaller than
// the minimum filler object, so we'll need to take away another card's
// worth to construct a filler object.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 263:

> 261:   }
> 262: 
> 263:   return usable_free / HeapWordSize;;

Extra semi-colon.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 266:

> 264: }
> 265: 
> 266: HeapWord* ShenandoahFreeSet::allocated_aligned_plab(size_t size, ShenandoahAllocRequest& req, ShenandoahHeapRegion* r) {

Let's write a documentation comment:

// Given a size argument, which is a multiple of card size, a request struct
// for a PLAB, and an old region, return a pointer to the allocated space for
// a PLAB which is card-aligned and where any remaining shard in the region
// has been suitably filled by a filler object.
// It is assumed (and assertion-checked) that such an allocation is always possible.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 266:

> 264: }
> 265: 
> 266: HeapWord* ShenandoahFreeSet::allocated_aligned_plab(size_t size, ShenandoahAllocRequest& req, ShenandoahHeapRegion* r) {

Did you really want the method to be called `allocated_...` or `allocate_...`.

I'd think the imperative verb form works better here, and I assume the extra `d` was an unintended typo?

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 272:

> 270:   assert(size % CardTable::card_size_in_words() == 0, "size must be multiple of card table size, was " SIZE_FORMAT, size);
> 271: 
> 272:   HeapWord* result = r->allocate_aligned(size, req, CardTable::card_size());

Note that `allocate_aligned` takes a bunch of safety measures that are, in this particular caller's case, redundant ad wasted. Since allocation paths must be made as efficient as possible, we should (in the future) try and get these to be more efficient and avoid such redundancy where possible.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 275:

> 273:   assert(result != nullptr, "Allocation cannot fail");
> 274:   assert(r->top() <= r->end(), "Allocation cannot span end of region");
> 275:   assert(req.actual_size() % CardTable::card_size_in_words() == 0, "PLAB size must be multiple of card size");

We've already checked at line 270 that `size % CardTable::card_size_in_words() == 0`. I think we can check a stronger condition here, i.e.:

assert(req.actual_size() == size, "Unexpected actual_size()");

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 323:

> 321:     if (req.type() == ShenandoahAllocRequest::_alloc_plab) {
> 322:       // Need to assure that plabs are aligned on multiple of card region.
> 323:       // Since we have Elastic TLABs, align size up.  This is consistent with aligning min_size up.

I's just say:
// Since we are using ElasticTLABs, we align the allocation up to card sizes/boundaries.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 325:

> 323:       // Since we have Elastic TLABs, align size up.  This is consistent with aligning min_size up.
> 324:       size_t usable_free = get_usable_free_words(r->free());
> 325:       size_t size = align_up(req.size(), CardTable::card_size_in_words());

For consistency, name this variable `adjusted_size` ~~or name the next variable just `min_size`~~.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 333:

> 331: 
> 332:       if (size >= adjusted_min_size) {
> 333:         result = allocated_aligned_plab(size, req, r);

`allocate_..`

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 338:

> 336:     } else {
> 337:       // This is a GCLAB or a TLAB allocation
> 338:       size_t size = req.size();

I prefer `adjusted_size` here too, but ok if you want to leave it this way (there are adjustments that happen below).

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 339:

> 337:       // This is a GCLAB or a TLAB allocation
> 338:       size_t size = req.size();
> 339:       size_t free = align_down(r->free() >> LogHeapWordSize, MinObjAlignment);

Not code that you wrote, but still ...

Why do we need this alignment downward? It sounds like this would want to be an assertion instead:

size_t free_words = r->free() >> LogHeapWordSize;
assert(free_words == align_down(free_words, MinObjAlignment), "Should always be min obj aligned");


Or may be I am missing something here. In which case a suitable documentation comment would be useful.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 358:

> 356:     size_t usable_free = get_usable_free_words(r->free());
> 357:     if (size <= usable_free) {
> 358:       result = allocated_aligned_plab(size, req, r);

`allocate_..`

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 373:

> 371:     // Allocation successful, bump stats:
> 372:     if (req.is_mutator_alloc()) {
> 373:       increase_used(req.actual_size() * HeapWordSize);

There is leakage of abstraction happening here. I'd prefer if the adjustments of heap use occurred when the space was allocated by the region. That avoids accounting errors because of piecemeal adjustments scattered along the allocation paths here.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 409:

> 407:     if (req.is_mutator_alloc()) {
> 408:       assert(req.is_young(), "Mutator allocations always come from young generation.");
> 409:       generation->increase_used(size * HeapWordSize);

Where is the generation's use being adjusted now?

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 52:

> 50:   bool is_collector_free(size_t idx) const;
> 51: 
> 52:   HeapWord* try_allocate_in(ShenandoahHeapRegion* region, ShenandoahAllocRequest& req, bool& in_new_region);

Again, this isn't your set of changes, so this is a more general comment that doesn't necessarily need specific action here, except perhaps for the methods that you touched.

Here and below, there are a bunch of allocate methods. If I look at this private set of methods, without tracing through their uses, it's a bit difficult to know where each should be used and whether some of these might call others (and if so, whose responsibility is what, say wrt keeping/updating stats to avoid double counting or forgetting to count).

May be expressing this in the form of some documentation comments will help communicate the high-level structure here. In general, it's always a good idea for header files to include a brief comment on the purpose/intent of a method to aid understanding and future maintainability.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 719:

> 717: // * The sum of a generation's regions::used == generation::used
> 718: // * The sum of a generation's humongous regions::free == generation::humongous_waste
> 719: // These invariants are checked by the verifier on safepoints.

Which safepoints? (I presume not all, so better to be specific: may be "at safepoints for GC."

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 727:

> 725: // * The bottom of a PLAB must be aligned on card size. In some cases this will
> 726: //   require padding in front of the PLAB (a filler object). Because this padding
> 727: //   is included in the region's used memory we include the padding in the accounting.

... in the accounting as waste.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 730:

> 728: // * Mutator allocations are used to compute an allocation rate. They are also
> 729: //   sent to the Pacer for those purposes.
> 730: // * There are three sources of waste:

In the comment, please number the lines (the 3 sources below).

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 742:

> 740:     assert(wasted_bytes == 0 || req.type() == ShenandoahAllocRequest::_alloc_plab, "Only PLABs have waste");
> 741:     generation->increase_used(actual_bytes + wasted_bytes);
> 742:     increase_used(actual_bytes + wasted_bytes);

So here, waste is included into used for both heap & generation.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 746:

> 744:     assert(req.is_mutator_alloc(), "Expected mutator alloc here");
> 745:     // padding and actual size both count towards allocation counter
> 746:     generation->increase_allocated(actual_bytes + wasted_bytes);

Here waste is included into allocation, as well as used for heap & generation below.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 779:

> 777:   if (ShenandoahPacing) {
> 778:     control_thread()->pacing_notify_alloc(words);
> 779:     pacer()->claim_for_alloc(waste, true);

So, was this a bug in the original code in that we if we generated any waste one would incur a tax on the original allocation twice, rather than only on the waste and cause more stalls than necessary? Or was it the case previously that one was intentionally taxing twice on the allocation for any amount of waste? 🤔

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 228:

> 226: public:
> 227:   void increase_used(const ShenandoahAllocRequest& req);
> 228:   void increase_used(size_t bytes);

Do we need both versions?

-------------

Changes requested by ysr (Author).

PR Review: https://git.openjdk.org/shenandoah/pull/260#pullrequestreview-1389076107
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169354930
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169355293
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170550784
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170541638
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170540755
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170544401
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170566357
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170591072
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170576548
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170574940
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170593918
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170522912
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170591588
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170580004
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170586286
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170591806
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170600070
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1170600880
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169385739
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169363805
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169364545
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169366252
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169381032
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169381232
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169411382
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1169358437

More information about the shenandoah-dev mailing list