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

[Tony Printezis]

Bengt and Ramki,

Thanks for your thoughts, which I agree with. I'll go ahead and push 
this with the PRODUCT_RETURN version given that it is consistent with 
what we've been doing so far, as both Ramki and Bengt suggested. And, 
yes, it'd be nice to revisit this in the future. FWIW, I'd like to see 
the whole of assert / product / not product / debug only and friends 
macros revisited given that there's a lot of redundancy between them and 
we use them inconsistently throughout the codebase.

Tony

On 4/29/2011 2:05 AM, Bengt Rutisson wrote:
>
> Ramki,
>
> I totally agree with you. Most important is to have some kind of 
> general guideline so that we can start working towards a common way of 
> implementing non-product code.
>
> I see your (and Tony's) point about PRODUCT_RETURN having been the way 
> to do this. However, I don't fully appreciate the "clutter argument" 
> against using a macro at the call site.
>
> To me the line "force_overflow()->update()" below is clutter if I am 
> debugging this code from a product build since it does not do 
> anything. But from just going through this code it is not clear that 
> it is clutter. I actually have to go and understand what 
> force_overflow() does. So, changing the line to 
> "NOT_PRODUCT(force_overflow()->update())" is actually, IMHO, adding 
> information - not adding clutter. The clutter is already there. It 
> just doesn't look like clutter. (Sorry for getting all philosophical 
> here :-) )
>
> if (task_num == 0) {
>   clear_marking_state(concurrent() /* clear_overflow */);
>   force_overflow()->update();
>
>   if (PrintGC) {
>     gclog_or_tty->date_stamp(PrintGCDateStamps);
>     gclog_or_tty->stamp(PrintGCTimeStamps);
>     gclog_or_tty->print_cr("[GC concurrent-mark-reset-for-overflow]");
>   }
> }
>
> Anyway, Tony, since we don't have any common rule for this, or maybe 
> the rule is even to use PRODUCT_RETURN, I don't think you have to 
> re-write your code for this change. I am more interested in the 
> general direction that we will go in.
>
> Bengt
>
>
> On 2011-04-29 01:08, Y. S. Ramakrishna wrote:
>> I'm OK with either. The general question
>> (not the specific one in regards to this
>> specific changeset) would benefit from
>> input from the bigger hotspot-dev alias
>> (for reasons of global consistency, as was
>> pointed out).
>>
>> I agree that having a clear discriminator at the
>> call-site definitely aids clarity. The clutter from
>> the macro perhaps offsets that a bit -- but the clutter
>> itself is the "signal" here, even if that may sound a
>> bit oxymoronic, so  may be there's an aesthetic balancing
>> act here to do, i suspect. So global style guidelines
>> will need to dictate behaviour, because as everyone noted
>> consistency (or in general, a monotonic move towards
>> consistency) would be good to keep the system from
>> going into an oscillatory state.
>>
>> Historically, one has of course favoured the PRODUCT_RETURN*
>> form for historical reasons, but it's a good
>> idea to revisit historical norms every once in a
>> while -- as long as such revision is consistent and
>> global, so that the system as a whole advances towards the
>> desired goal.
>>
>> I'll now get off the soap-box :-)
>> -- ramki
>>
>> On 04/28/11 12:57, Tony Printezis wrote:
>> ...
>>> I do see the point you and Jesper (he made in his subsequent e-mail) 
>>> and, of course :-), we're generally not consistent in taking one 
>>> approach over the other in HotSpot. Having said this, this is the 
>>> sort of situation for which the PRODUCT_RETURN macro was introduced 
>>> in the first place. I personally like using PRODUCT_RETURN since it 
>>> cuts down the clutter. Compare:
>>>
>>> void foo() PRODUCT_RETURN;
>>>
>>> #ifndef PRODUCT
>>> void foo() {
>>>  ...
>>> }
>>> #endif
>>>
>>> foo();
>>>
>>> with:
>>>
>>> NOT_PRODUCT(void foo();)
>>>
>>> #ifndef PRODUCT
>>> void foo() {
>>>  ...
>>> }
>>> #endif
>>>
>>> NOT_PRODUCT(foo();)
>>>
>>>
>>> But I'd be OK with the latter for the reasons you and Jesper brought 
>>> up. Anyone else in group have a strong opinion one way or another?
>>>
>>> Tony
>