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

[Bengt Rutisson]

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