RFR (XXS): JDK-8138610 add assert to ThreadLocalAllocBuffer::clear_before_allocation to check the storage above top

[Thomas Schatzl]

Hi Joe,

On Mon, 2017-02-13 at 13:57 -0500, Joseph Provino wrote:
> Please review this one line change:
> 
> CR: JDK-8138610 <https://bugs.openjdk.java.net/browse/JDK-8138610>
> add assert to ThreadLocalAllocBuffer::clear_before_allocation to
> check the storage above top
> 
> Webrev: http://cr.openjdk.java.net/~jprovino/8138610/webrev.00
> 
> Tests:  Passes JPRT.

  I am not sure that the assert is correct.

  45   assert(!ZapUnusedHeapArea || _reserve_for_allocation_prefetch >
0 || 
  46     top() == NULL || *(intptr_t*)top() != 0, "overzeroing
detected"); 

Maybe I do not understand the related code here, but some questions:

 - did you run jprt with -XX:-ZapUnusedHeapArea? Because this is a
develop option that is true in debug mode, i.e. that assert will never
be checked without explicitly disabling the ZapUnusedHeapArea flag.

 - if ZeroTLAB (another option that is disabled by default) is enabled,
that assert will likely fail because it, as the switch indicates, zeros
the entire tlab. I.e. if that code is called before top() reaches
end(), the contents are guaranteed to be zero.

 - it is not clear to me too why if _reserve_for_allocation_prefetch is
greater than zero, we do not want to do this check. Even with
_reserve_for_allocation_prefetch == 0, there is some alignment reserve
at the end of the TLAB that could be "overzeroed".

 - neither it is clear to me that the heap after the last object, e.g.
in the alignment reserve always contains non-zero data (e.g. if calling
this with top() == end()).

 - related to this, I looked at the CR, and it has a cryptic comment
that adds another question: what does "uninitialized" mean? (We
typically do not initialize the TLAB contents at all).

 - why does the value at top() should not be zero, and not for example
badHeapWordVal? (Which the TLAB is initialized with
-XX:+ZapUnusedHeapArea). I.e. could that check be made more specific?

 - why does the message of the assert talk about "overzeroing"? Does
this refer to the object initialization, or -XX:+ZeroTLAB?

 - jprt only testing is not sufficient due to resman.

The patch as is seems to be some random debug assert that just worked
for the specific case the original author of the change had been
investigating at that time.

I recommend asking the original author for feedback on these questions
before continuing.

Thanks,
  Thomas