RFR (S): 8143215: gcc 4.1.2: fix three issues breaking the build.

[Kim Barrett]

On Dec 14, 2015, at 4:43 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> 
> Hi Kim and Goetz,
> 
> On 2015-12-13 00:02, Kim Barrett wrote:
>> On Dec 11, 2015, at 5:52 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>> diff -r 73e267ddd93d src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>>> --- a/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp     Wed Nov 18 11:31:59 2015 +0100
>>> +++ b/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp     Fri Dec 11 11:42:52 2015 +0100
>>> @@ -2731,7 +2731,7 @@
>>>   // Not MT-safe; so do not pass around these StackObj's
>>>   // where they may be accessed by other threads.
>>>   jlong wallclock_millis() {
>>> -    return TimeHelper::counter_to_millis(os::elapsed_counter() - _trace_time.start_time());
>>> +    return (jlong)TimeHelper::counter_to_millis(os::elapsed_counter() - _trace_time.start_time());
>>>   }
>>> };
>> This conversion was introduced by the recently pushed 8145092: Use Unified Logging for the GC logging.
>> 
>> The only caller of wallclock_millis compares the result with CMSMaxAbortablePrecleanTime (an intx).
>> 
>> So I think we’ve always had a bit of a type mismatch here.
>> 
>> I’m not sure what to do about this one yet, but adding a cast doesn’t seem like the right solution to me.
> 
> I would suggest changing CMSMaxAbortablePrecleanTime to a double. It may not be the cleanest way to handle the flag but having it as an intx is also not very clean. Negative values won't make sense. Changing it to a double won't make the situation much worse than it is now and it will simplify the code.

Unfortunately, the syntax for floating point options requires the decimal point and fraction part; integer
syntax is rejected.  So existing uses of that option would be broken by changing its type.
CMSMaxAbortablePrecleanTime is a product option, making it not so easy to make such a change.


>>> diff -r 73e267ddd93d src/share/vm/gc/g1/g1CollectedHeap.cpp
>>> --- a/src/share/vm/gc/g1/g1CollectedHeap.cpp    Wed Nov 18 11:31:59 2015 +0100
>>> +++ b/src/share/vm/gc/g1/g1CollectedHeap.cpp    Fri Dec 11 11:42:52 2015 +0100
>>> @@ -3621,7 +3621,7 @@
>>>     log_info(gc)("To-space exhausted");
>>>   }
>>> 
>>> -  double pause_time_sec = TimeHelper::counter_to_seconds(pause_time_counter);
>>> +  double pause_time_sec = TimeHelper::counter_to_seconds((jlong)pause_time_counter);
>>>   g1_policy()->print_phases(pause_time_sec);
>>> 
>>>   g1_policy()->print_detailed_heap_transition();
>> Adding a cast is not the proper fix here.
>> 
>> The problem is that log_gc_footer’s double pause_time_counter argument ought to itself be a jlong.
>> The caller (do_collection_pause_at_safepoint) contains
>> 
>>   double pause_start_counter = os::elapsed_counter();
>>   …
>>   log_gc_footer(os::elapsed_counter() - pause_start_counter);
>> 
>> elapsed_counter returns a jlong.  So pause_start_counter ought to be a jlong too, and fixing the
>> parameter type for log_gc_footer would then make everything work out.
>> 
>> This appears to be a mistake introduced by the recently pushed 8145092: Use Unified Logging for the GC logging.
>> 
> 
> Yes, this is a mistake in the push for the unified logging work. I created a patch and sent out a new review request to fix it.
> 
> RFR: JDK-8145303: Clean up the units for log_gc_footer
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-December/015903.html
> 
> http://cr.openjdk.java.net/~brutisso/8145303/webrev.00/

Thanks for taking care of this.