RFR: Usage tracking cleanup [v2]

[Y. Srinivas Ramakrishna]

On Wed, 19 Apr 2023 00:08:01 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> 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.
>
> Some of these checks are still required when ElasticTLABs is disabled.

Ah, you are right, I had missed the subtlety in the inelastic PLAB arm in my first reading in how the value from `usable_free_words()` is used.

>> 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.
>
> Is this just making sure there is enough free space for a minimum sized object?

I realize now that if the user runs with a larger than needed value for "MinObjAlignment" by setting it on the command-line, this will respect that request. This code is fine. Apologies for my confusion. I had somehow assumed that if MinObjAignment is set then the heap (regions) always maintain the invariant that object space is allocated in chunks that are always aligned to that request. Perhaps that is not true in general. It's been a while since I have been in this code (although I am sure @rkennke has been recently on account of Lilliput!)

>> 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.
>
> No, here waste is only used to update the allocation rate and below it is only used for similar purposes with the pacer. It isn't included in the `used` memory for mutator allocations.

OK, that makes sense. Sorry for my confusion, and thank you for the related documentation you've added to underline this!

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1171974973
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1171981330
PR Review Comment: https://git.openjdk.org/shenandoah/pull/260#discussion_r1171669947