RFR (XXL): 8208390: G1 should always use ticks as internal time base instead of double

[Thomas Schatzl]

Hi all,

   after some internal discussion there were some ideas that would 
potentially cause larger changes to this changeset.

Please hold off further reviewing for now.

Thanks,
   Thomas

On 28.04.20 10:14, Thomas Schatzl wrote:
> Hi all,
> 
>    can I have reviews for this (tedious to look at) change that moves 
> G1's time base from double's to Ticks/Tickspan?
> 
> Some comments:
> 
> - this covers only immediate and immediately related changes to use 
> Ticks/Tickspan in G1GCPhaseTimes. For various reasons some components 
> still use doubles (G1Analytics, MMU, ...).
> 
> - only minor other cleanup has been performed apart from exceptions 
> below. There is a big follow-up rename to also remove the _sec/_ms/* 
> suffixes currently available for reference to rename variables, methods 
> etc. at http://cr.openjdk.java.net/~tschatzl/8208390/webrev.rename/. 
> Will file a separate CR for that.
> 
> - also a few obvious bugs or cleanups for incomprehensible code (in that 
> it wasn't obvious what G1 was trying to achieve) have been filed or 
> existing amended.
> 
> - G1GCPhaseTimes has been a bit sloppy about handling points in time vs. 
> durations, so an explicit distinction had to be made in the change.
> 
> - for support of G1CollectedHeap::millis_since_last_gc() G1 previously 
> reused G1Policy::_collection_pause_end; now that (at least 
> superficially) _collection_pause_end and the value returned by 
> G1CollectedHeap::millis_since_last_gc() use a different time source 
> (Ticks vs. os::javaTimeNanos()) a separate variable has been introduced 
> (_collection_pause_end_javaTime).
> 
> Not sure why that millis_since_last_gc() calculation should be part of 
> G1Policy (it's unused there), particularly now, but I kept it there. 
> Will file a CR for that.
> 
> - this change necessitated some improvements to the Ticks/Tickspan 
> interface - cc'ed the jfr team to look at these too:
> 
>    - some conversion methods from SI time units (s, ms, us, ns) into the 
> internal (jlong) format.
> 
>    - shortened the conversion methods from internal units to SI time 
> units to SI abbreviations s/ms/us/ns because particularly the latter 
> that are commonly used for printing are very long.
> 
>    - all these methods that convert to SI time units now return the 
> "raw" double result from the method. Almost all of the uses are timing 
> critical (99.99% just printing), and the single other use already did FP 
> calculations anyway now and then.
> 
>    - I avoided cluttering the Ticks/Tickspan interface with more 
> methods: G1 does some statistics with Ticks/Timespan, so actually some 
> more arithmetic operations would have been nice. These methods have been 
> implemented using template specializations in WorkerDataArray without 
> adding new methods in Ticks/Tickspan.
> 
> - there are no helper functions/uses for WorkerDataArray<double> any 
> more (I changed the single one in Shenandoah because it was not worth 
> keeping imho so I just converted to Tickspan).
> 
> - there is one particular change in workerDataArray.inline.hpp that 
> required me to manually expand some MAX2/MIN2 macros in a template 
> method because otherwise the solaris studio compiler generated bad code 
> on SPARC (caught by a gtest). I will file a CR to fix this when/if the 
> compiler or the solaris situation changes.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8208390
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8208390/webrev/
> Testing:
> hs-tier1-5, eyeballing results, building and testing Shenandoah locally, 
> eyeballing results afaiu them.
> 
> Thanks,
>    Thomas