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

[Bengt Rutisson]

Hi Goetz,

On 2015-12-14 12:19, Lindenmaier, Goetz wrote:
> Hi,
>
> I removed the change to g1CollectedHeap.cpp
> ("8145303: Clean up the units for log_gc_footer" will fix this)
> and changed the flag to 'double'. New webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8143215-gcc412/webrev.05/

This looks good to me, but I haven't been following this thread closely 
so it would be good if Kim and Thomas take a look too.

Thanks,
Bengt

>
> Best regards,
>    Goetz.
>
>
>> -----Original Message-----
>> From: Bengt Rutisson [mailto:bengt.rutisson at oracle.com]
>> Sent: Montag, 14. Dezember 2015 10:44
>> To: Kim Barrett <kim.barrett at oracle.com>; Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.com>
>> Cc: hotspot-gc-dev at openjdk.java.net
>> Subject: Re: RFR (S): 8143215: gcc 4.1.2: fix three issues breaking the build.
>>
>>
>> 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.
>>
>> So, I would suggest:
>>
>> diff --git a/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>> b/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>> --- a/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>> +++ b/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>> @@ -2733,1 +2733,1 @@
>> -  jlong wallclock_millis() {
>> +  double wallclock_millis() {
>> diff --git a/src/share/vm/runtime/globals.hpp
>> b/src/share/vm/runtime/globals.hpp
>> --- a/src/share/vm/runtime/globals.hpp
>> +++ b/src/share/vm/runtime/globals.hpp
>> @@ -1836,1 +1836,1 @@
>> -  product(intx, CMSMaxAbortablePrecleanTime,
>> 5000,                          \
>> +  product(double, CMSMaxAbortablePrecleanTime,
>> 5000,                        \
>>
>>
>>>> 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,
>> Bengt