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

[Bengt Rutisson]

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