RFR (S): 8043243: convert SCAN_AND_FORWARD, SCAN_AND_ADJUST_POINTERS, SCAN_AND_COMPACT macros to methods

[Mikael Gerdin]

Marcus,

On Monday 08 September 2014 13.17.38 Marcus Larsson wrote:
> Hi Stefan,
> 
> On 08/28/2014 02:14 PM, Stefan Karlsson wrote:
> > Hi Marcus,
> > 
> > On 28/08/14 13:46, Marcus Larsson wrote:
> >> Hi,
> >> 
> >> I would like reviews for the following patch converting the
> >> SCAN_AND_* macros into (inline) methods.
> >> 
> >> Short summary:
> >> The change is based on the demacroify.patch (bug attachment), using a
> >> proxy class to enable different functions for obj_size and similar to
> >> be used for different types of Spaces. Each type of space has a proxy
> >> defining which methods should be used in the scan_and_* functions.
> >> Made no changes to the actual code in the macros, except for
> >> replacing a multi-line debug_only(...) with #ifdef ASSERT ... #endif.
> > 
> > Some background to this change. I wrote it as PoC to see if we could
> > get rid of these large macros. At that time there werw concerns raised
> > that some compilers might not be able to inline the template
> > functions. I checked the code generated by gcc and it was inlined as
> > expected, but I didn't check the code from the other compilers.
> > 
> > Are the list of benchmarks below performance runs? Was it done on
> > different platforms? Did you measure the changed phases
> > (prepare_for_compation, adjust_pointers, etc.) or did you measure the
> > benchmark score?
> 
> The measurements were performance runs, but I only measured benchmark
> score. I reran the benchmarks, this time to also verify/look at overall
> GC times and the different phase times. I could find no significant
> changes in these times on any of the tested platforms (Solaris, Linux,
> Windows).
> 
> >> Webrev:
> >> http://cr.openjdk.java.net/~brutisso/webrev-8043243/
> > 
> > Looking at the patch now, I wonder if we shouldn't get rid of the
> > instance variables in the *SpaceProxy classes and instead pass down
> > 'this' from the call sites? And make these classes AllStatic.
> 
> Made the proxies AllStatic (reruns described above used this new version).
> Also followed Mikael's suggestion and made explicit calls to the known
> member functions in proxies for CompactibleFreeListSpace and
> G1OffsetTableContigSpace.
> 
> New webrev:
> http://cr.openjdk.java.net/~brutisso/webrev-8043243v2/

Looks good.
/Mikael

> 
> Thanks,
> Marcus
> 
> > thanks,
> > StefanK
> > 
> >> Bug:
> >> https://bugs.openjdk.java.net/browse/JDK-8043243
> >> 
> >> Testing:
> >> jprt without problems
> >> SPECjbb2013, SPECjbb2005, SPECjvm2008 - no significant changes
> >> 
> >> Thanks,
> >> Marcus