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

[Lindenmaier, Goetz]

Hi,

Seeing Mikaels mail, I did not convert the computation in 
g1IHOPControl.cpp:145 ff.
I changed though the computations in gcTraceSend.cpp.
Please check whether I should move the division to 
be performed before the multiplication.

See more comments inline below.

New webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8143215-gcc412/webrev.03/index.html

Best regards,
  Goetz.

> -----Original Message-----
> From: Kim Barrett [mailto:kim.barrett at oracle.com]
> Sent: Thursday, December 10, 2015 1:24 AM
> To: 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.
> 
> 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?
There is a line feed missing at the end.  That's not visible in the 
diffs, look at the patch itself.

> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1CollectorPolicy.cpp
>  538   return
> update_young_list_max_and_target_length((size_t)get_new_prediction(_r
> s_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!
I introduced get_new_size_prediction() as Mikael said.

> ------------------------------------------------------------------------------
> 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 just added the cast, as Mikael said changing this should be a different changeset.

> 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 = ...
Good point, fixed.

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

> ------------------------------------------------------------------------------