CRR: 7034139: G1: assert(Thread::current()->is_ConcurrentGC_thread()) failed: only a conc GC thread can call this (S)

[Jesper Wilhelmsson]

On 2011-04-28 09:35, Bengt Rutisson wrote:
>
> Tony,
>
> I think this fix looks good.
>
> I like that you have a way of testing this, but if it is possible I would like
> to see if it can be made more obvious in the code what parts are "real" code
> and what parts are there just for testing.
>
> For example, I think I would prefer that concurrentMark.cpp had calls like:
>
> DEBUG_ONLY(force_overflow()->update());
>
> rather than having this in concurrentMark.hpp:
>
> void update() PRODUCT_RETURN;
>
> That way someone browsing the code in concurrentMark.cpp can quickly see the
> difference between test code and product code. I realize that this might be a
> matter of taste, so I'm ok with the way it is now as well.

I agree with Bengt that it would be preferred to make it visible at the call 
site that this call is for debugging only. I don't think this is just a matter 
of taste, it is also a matter of clarity and consistency.
/Jesper


>
> Bengt
>
> On 2011-04-23 00:58, Tony Printezis wrote:
>> Thanks to John Cuthbertson for looking at this. I took his advice and I'm
>> going to disable the forced overflow by default (by setting the default
>> parameter to 0), but leave the code in as it's helpful. Latest version here:
>>
>> http://cr.openjdk.java.net/~tonyp/7034139/webrev.2/
>>
>> Tony
>>
>> Tony Printezis wrote:
>>> Hi all,
>>>
>>> I'd still like a couple of code reviews for this. Here's the latest version
>>> (I only rephrased a couple of comments, so if you're looking at the earlier
>>> version already you can ignore this one):
>>>
>>> http://cr.openjdk.java.net/~tonyp/7034139/webrev.1/
>>>
>>> Tony
>>>
>>> Tony Printezis wrote:
>>>> Hi,
>>>>
>>>> Could I get a couple of people to look at this? (I'd like to push this this
>>>> week if possible)
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7034139/webrev.0/
>>>>
>>>> The actual fix is reasonably small (leave / join the SuspendibleThreadSet
>>>> only if we are in concurrent mode). Most of the changes are new
>>>> infrastructure to cause a fixed number of overflows during marking (in
>>>> non-product builds of course) to stress the overflow code. This was the
>>>> only way I could reliably reproduce the failure. This did uncover a couple
>>>> of extra issues which I also fixed:
>>>>
>>>> - If we overflow during remark we should not actually deal with it during
>>>> remark but we should abort the remark pause and restart a concurrent mark
>>>> phase. For some reason we were not doing that. I fixed that (for this I had
>>>> to ensure that the overflow flag is not cleared when we exit the
>>>> do_marking_step() method).
>>>> - Because we were clearing the overflow, it was also possible that the
>>>> workers would deadlock (for that to happen a worker had to finish handling
>>>> one overflow and immediately raise another one, so it was highly unlikely
>>>> to occur in prcatice; good to find it and eliminate it though).
>>>>
>>>> I've already tested it, I'll run more tests overnight.
>>>>
>>>> Tony
>>>
>