RFR: 8267579: Thread::cooked_allocated_bytes() hits assert(left >= right) failed: avoid underflow [v2]

[Stefan Karlsson]

On Mon, 7 Jun 2021 14:15:56 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> This is a workaround to avoid hitting the assert that was recently added to pointer_delta(). 
>> 
>> The implementation of cooked_allocated_bytes() is perhaps questionable, in that it reads tlab pointers optimistically. However, the functionality has been in place for a long time, and the impact of changing its behavior more substantially is unknown at this time, hence this defensive workaround to reduce noise and problems seen in general testing.
>> 
>> Testing: tier1, tier2, tier6, tier8
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   read once

> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-jfr-dev](mailto:hotspot-jfr-dev at mail.openjdk.java.net):_
> 
> On 10/06/2021 4:55 pm, Stefan Karlsson wrote:
> 
> > On Thu, 10 Jun 2021 04:35:50 GMT, David Holmes <david.holmes at oracle.com> wrote:
> > > > I'm not sure what you are asking, but Atomic::load implements the "relaxed" semantics equivalent to using MO_RELAXED in the Access API.
> > > 
> > > 
> > > I see this terminology is used in some of the GC code but I was not
> > > aware of its existence.
> > 
> > 
> > This is not a GC-specific terminology, but a C++11 terminology:
> > https://en.cppreference.com/w/cpp/atomic/memory_order
> 
> I meant in the context of the hotspot sources and its use in method names.

I guess the other parts of HotSpot needs to get on board with this terminology now that we have gone to C++11.

> 
> > > I also see nothing in atomic.hpp that states
> > > that Atomic::load/store implement memory_order_relaxed ??
> > 
> > 
> > In atomic.hpp there are references to relaxed at the top of the file:
> > enum atomic_memory_order {
> > // The modes that align with C++11 are intended to
> > // follow the same semantics.
> > memory_order_relaxed = 0,
> > memory_order_acquire = 2,
> > memory_order_release = 3,
> > memory_order_acq_rel = 4,
> > // Strong two-way memory barrier.
> > memory_order_conservative = 8
> > };
> > But you are right, nothing here helps the reader understand that Atomic::load implies relaxed. This needs to be better documented.
> 
> What am I missing - I can't see any memory barrier related instructions
> associated with basic Atomic::load and store. Granted store needs
> nothing on TSO and on non-TSO the barrier may be implicit in whatever
> platform intrinsic is used; but loads would also need explicit barriers
> to prevent reordering ??

AFAIK, the code was written prior to C++11 and nothing else is needed on our currently supported platforms. Though I've heard stories about some platforms that probably would have to fix this. I've also heard @fisk argue that we should replace the Atomic::load implementation with the compiler's version of relaxed atomic loads.

> 
> David
> -----

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

PR: https://git.openjdk.java.net/jdk/pull/4363