RFR: 8329134: Reconsider TLAB zapping

[Stefan Karlsson]

On Tue, 26 Mar 2024 21:08:16 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> We zap the entire TLAB on initial allocation (`MemAllocator::mem_allocate_inside_tlab_slow`), and then also rezap the object contents when object is allocated from the TLAB (`ThreadLocalAllocBuffer::allocate`). The second part seems excessive, given the TLAB is already fully zapped.
> 
> There is also no way to disable this zapping, like you would in other places with the relevant Zap* flags.
> 
> Fixing both these issues allows to improve fastdebug tests performance, e.g. in jcstress. 
> 
> It also allows to remove the related Zero kludge.
> 
> Additional testing:
>  - [ ] Linux AArch64 server fastdebug, `all` tests 
>  - [x] MacOS AArch64 Zero fastdebug, `bootcycle-images` pass

This looks like a worth-while change. I listed a couple of nits that I think would be nice to clean up.

src/hotspot/share/gc/shared/memAllocator.cpp line 323:

> 321:     Copy::zero_to_words(mem, allocation._allocated_tlab_size);
> 322:   } else if (ZapTLAB) {
> 323:     // ...and zap just allocated TLAB.

Could you add a blank line after this, so that the two separate comments don't get bunched together?

src/hotspot/share/gc/shared/threadLocalAllocBuffer.inline.hpp line 43:

> 41:   if (pointer_delta(end(), obj) >= size) {
> 42:     // successful thread-local allocation
> 43: #ifdef ASSERT

Could you add a blank line after this, so that the two separate comments don't get bunched together?

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

> 882:     Copy::zero_to_words(gclab_buf, actual_size);
> 883:   } else if (ZapTLAB) {
> 884:     // ...and zap just allocated TLAB.

Could you add a blank line after this, so that the two separate comments don't get bunched together?

src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 1996:

> 1994:               // Initialize object field block.
> 1995:               // If TLAB was pre-zeroed, we can skip this path.
> 1996:               if (!ZeroTLAB) {

"skip this path" refers to the code inside the if block. I think it would be nicer to just place a comment in that block, so that we don't have to figure out what "this path" means. Maybe something like:


if (!ZeroTLAB) {
  // The TLAB was not pre-zeroed, so clear the memory here.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18500#pullrequestreview-1963374309
PR Review Comment: https://git.openjdk.org/jdk/pull/18500#discussion_r1541141628
PR Review Comment: https://git.openjdk.org/jdk/pull/18500#discussion_r1541142044
PR Review Comment: https://git.openjdk.org/jdk/pull/18500#discussion_r1541145082
PR Review Comment: https://git.openjdk.org/jdk/pull/18500#discussion_r1541159636