RFR: 8251158: Implementation of JEP 387: Elastic Metaspace [v11]

[Richard Reingruber]

On Tue, 6 Oct 2020 11:48:29 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Okay, I feel stupid now but I do not see it.
>> 
>> I see the non-atomicity, but not how it could cause a false positive assert here. I see that I could miss an assert,
>> but not the other way around.
>
> I will remove the asserts. These classes are scheduled to become general utility classes anyway in a followup RFE, so
> they will be revisited soon.

It is not as clear as I though. So no need at all to feel stupid.

> I will remove the asserts. These classes are scheduled to become general utility classes anyway in a followup RFE, so
> they will be revisited soon.

I'm ok with that.

For the sake of completeness: I'd still think false positives are possible. Please look at the following example:

`
   1	_initialized = 0;
   2	_c = 0;
   3
   4	Thread A                                  Thread B
   5
   6	                                          Atomic::inc(&_c)
   7
   8	                                          <membar StoreLoad|StoreStore>
   9
  10	                                          _initialized = 1;
  11
  12	while(_initialized == 0) busy_wait();
  13
  14	T old = Atomic::load_acquire(&_c);
  15
  16	assert(old >= 1, "underflow (" UINT64_FORMAT "-1)", (uint64_t)old);
  17
  18	<fence>
  19
  20	Atomic::dec(&_c);
`

Loading `old` at line 14 can be done speculatively before the loop, for instance at line 5. There is no barrier that
would prevent this. `Atomic::inc` and `Atomic::dec` both have a `<fence>` before and a `<membar StoreLoad|StoreStore>`
after the operation. Because of the effect of the `<fence>` in line 18 the `dec()` in line 20 cannot be done before the
while in line 12. An underflow is not possible. The assertion can fail though.

So probably you could fix the assertion by adding a fence before the load of the `old` value.

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

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