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

[Stefan Karlsson]

On 2014-09-08 13:17, 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.

StefanK

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