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

[Thomas Schatzl]

Hi Kim,

On Wed, 2015-12-09 at 19:23 -0500, Kim Barrett wrote:
> 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.
[...]
> ------------------------------------------------------------------------------ 
> 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!

Fwiw I would prefer the first option :)

> ------------------------------------------------------------------------------ 
> 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.

Integer/size_t overflow (on 32 bit platforms) for the first term. Now
you can argue that the FP conversion may incur lots of rounding too on
very large values of max_capacity().

It's probably better to use integer calculations throughout here. I have
not found a helper method that does that correctly without potential
overflow.

> [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.

It would be fine with me to change this as suggested, however it would
be great if that change would not be hidden in a fix-the-casts CR.

Thanks,
  Thomas