RFR(M/L): 7115356: assert(!m->was_never_executed()) failed

[Nils Eliasson]

I think Vladimir is right with his observation that JDK-8058564 is the 
actual cause.

The scenario I see is that the carry may be observed as 0 immediately 
after setting it (by the same thread).

Thread A running: InvocationCounter::set_carry()
Thread B running: InvocationCounter::set(State state)

  A reads _counter with carry 0
  B writes carry 1
  A writes _counter with carry 0
  B reads _counter to mask out carry, finds carry is 0
  B writes _counter with carry 0

Result: _counter ends up with the count from B but carry from A.

  - In my refactored version this turns into:

  A reads _counter, carry 0
  B reads _counter, carry 0
  A writes counter with carry 0 /  B writes counter with carry 1

We still might end up with loosing any update - but they will at least 
be internally consistent - if the flag is set - the count was reduced, 
and if it was not - the count is still high. However this scenario 
doesn't explain how the count gets to zero.

//Nils

On 2014-09-18 04:45, Vladimir Kozlov wrote:
> The clean up is nice.
> As David I want to see the cause description. I tried to find a cause 
> together with Rickard without much success.
>
> The assert we hit is next after 2 set_carry() calls:
>
>   mcs->invocation_counter()->set_carry();
>   mcs->backedge_counter()->set_carry();
>   assert(!m->was_never_executed(), "don't reset to 0 -- could be 
> mistaken for never-executed");
>
> where
>
> bool was_never_executed()  { return !was_executed_more_than(0); }
>
> The only code which can return 'false' from was_executed_more_than(0) 
> is next:
>
> return invocation_count() > 0;
>
> Failures happens without tiered compilation an on Windows only (?).
> In such case invocation_count() returns:
>
> return (mcs == NULL) ? 0 : mcs->invocation_counter()->count();
>
> You would assume that mcs != NULL because we just accessed it before 
> the assert.
>
> The one explanation is race during MethodCounters creation which Igor 
> is fixing (8058564). Which can produce 0 count.
>
> Vladimir
>
> On 9/17/14 7:05 AM, Nils Eliasson wrote:
>> Hi,
>>
>> Please review this change. It started out as a bug fix and grew with
>> some refactoring of invocationCounter.cpp/hpp and its uses.
>>
>> The original problem was that an assert in compilationPolicy failed
>> because a value that was just set could not be observed. Some code paths
>> read the composite counters a part at a time and may observe and write
>> inconsistent values when several threads are using them. While removing
>> a few unnecessary read and writes of the 'state' field, I noticed the
>> state and Action was actually never used. I removed it all and came up
>> with much simpler code.
>>
>> Major refactorizations:
>> 1) Rewritten invocationCounter methods to read entire counter,
>> manipulate and then write entire counter.
>> 2) Removed the state, and action - it was never used anywhere.
>> 3) Moved invocation counter logic from simpleThresholdPolicy into
>> invocationCounter.
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-7115356
>> webrev: http://cr.openjdk.java.net/~neliasso/7115356/webrev.06
>>
>> This code might affect the logCompilation and replay tools since it
>> changes the output. I will test and fix that before submitting. I will
>> also do some performance testing to make sure I haven't messed up the
>> counters in any way.
>>
>> Thanks,
>> Nils Eliasson