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

[Kim Barrett]

On Dec 3, 2015, at 8:55 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
> 
> I really would appreciate if we could get this fixed soon.
> Maybe we can submit my version and if we put a comment into
> 8135181 that points to this change, the questionable casts can be 
> identified later on, too.

I’m not sure that’s really worth the time.  Addressing 8135181 well is going to
require looking over a lot of existing code for these sorts of things anyway.  I
don’t think having a few specifically noted there is going to make much difference.

> The broken build hinders our work. I have to manually fix our nightbuilds all 
> the time, and my colleagues stomped into this and fixed it in their workspace
> locally.  Any webrev we prepare we have to patch with this change ...
> So I really would like to see this resolved.
> 
> The change is now pending since 19.11.

Thanks for your patience.  This set of proposed changes raised some more global
issues.  I’ve tried to capture some of the discussion of those in 8135181 comments,
with this change to go ahead (subject to detailed review), without being blocked while
discussion of those larger issues has time to bake.

> I rebased my webrev, which also contains some more casts
> for issues poping up since my first attempt to fix this:
> http://cr.openjdk.java.net/~goetz/webrevs/8143215-gcc412/webrev.01/
> (Kim, you've seen this before).

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/concurrentG1RefineThread.cpp

File is in both webrev.00 and webrev.01, but no changes in either?

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1CollectorPolicy.cpp
 538   return update_young_list_max_and_target_length((size_t)get_new_prediction(_rs_lengths_seq));

There are a couple of these conversions of get_new_prediction in this
change set, and several others pre-existing.  Rather than continuing
to scatter casts about, I think better would be to have a variant of
get_new_prediction that returns a size_t rather than a double, with
the conversion centralized there.

get_new_size_prediction?  get_predicted_size?  Let the bikeshedding
begin!

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1IHOPControl.cpp
 145   double safe_total_heap_percentage = MIN2((double)(_heap_reserve_percent + _heap_waste_percent), 100.0);
 146 
 147   return (size_t)MIN2(
 148     (G1CollectedHeap::heap()->max_capacity() * (100.0 - safe_total_heap_percentage) / 100.0),
 149     (_target_occupancy * (100.0 - (float)_heap_waste_percent) / 100.0)
 150     );

[Pre-existing]
It's not clear why line 145 involves floating point at all.

[New]
Why was a float cast added on line 149?

I looked at rewriting this to use only integer calculations, by first
dividing the LHS of the multiplication operations by 100, then
performing scaling. The absolute rounding error would be small, making
the relative rounding error likely insignificant. It obfuscates the
code a bit though.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1IHOPControl.cpp
 164       ((size_t)(pred_marking_time * pred_promotion_rate) +

Rather than inserting a cast into the middle of the computation, I
think I'd rather see a new variable introduced for the converted
value, e.g. size_t pred_promotion_size = ...

------------------------------------------------------------------------------  
src/share/vm/gc/g1/g1IHOPControl.cpp
 178 void G1AdaptiveIHOPControl::update_allocation_info(double allocation_time_s, size_t allocated_bytes,
 179                                                    size_t additional_buffer_size) {

Change was to add a line break before the last parameter.  Please, if
breaking up parameter lists like that, put all of the parameters on
separate lines.

------------------------------------------------------------------------------