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

[Kim Barrett]

On Oct 24, 2014, at 11:00 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
> 
>> src/share/vm/memory/space.hpp
>>  240   // Mark-sweep-compact support: all spaces can update pointers to objects
>>  241   // moving as a part of compaction.
>>  242   virtual void adjust_pointers();
>> 
>> Is this definition still used or needed?  If it is, then there may be
>> other problems.
>> 
> 
> Good catch. Saw this definition when I started on the bug but had forgotten about it now. I removed the definition and made the function pure virtual in the Space class instead.

Good.

>> I'm very close to saying "looks good other than the above".  However,
>> this bit
>> 
>> src/share/vm/memory/space.hpp
>> [in class CompactibleSpace]
>>  431   inline bool scanned_block_is_obj(const HeapWord* addr) const {
>>  432     // Perform virtual call. This is currently not a problem since this
>>  433     // function is only used in an assert (from scan_and_adjust_pointers).
>>  434     return block_is_obj(addr);
>>  435   }
>> 
>> is bothering me.
>> 
>> […]
>> 
>> Also, I would like there to be a clear and simple theory (preferably
>> actually written out as a comment) regarding where various definitions
>> need to appear in the class hierarchy.  Something along the lines of
>> […]
> 
> Added some comments like these for the CompactibleSpace class, which will hopefully better explain the situation. Let me know if this is good or if you want to add/change anything.

Yes, that looks good.  Thanks.

>> The smallest code change from the current webrev version is to
>> eliminate the dependency on scanned_block_is_obj() by
>> scan_and_adjust_pointers(), and just change the assert to explicitly
>> and directly call the virtual block_is_obj() function. 

> This seems to me like the most straight forward way of solving this. It also makes more sense that the verification uses the logic dictated by the actual space class, rather than the sometimes simplified scanned_block_is_obj check. Removed the CompactibleSpace::scanned_block_is_obj and changed the assert.

[Didn’t like my ScanSupport helper class?  Well, it was an interesting little exercise for me anyway.]

OK.

>> - Hides the auxiliary functions from external access.  [The current
>> webrev exposes them publicly, though I'm not sure it needs to.]
> 
> Seeing how the auxiliary functions are meant exclusively for the scan_and_* functions it makes more sense not to expose them. Updated the patch to make these functions private, with the necessary friend declarations for scan_and_* to still be able to call them.

Good.

> New Webrev:
> http://cr.openjdk.java.net/~mlarsson/8043243/webrev.01/
> 
> Incremental:
> http://cr.openjdk.java.net/~mlarsson/8043243/webrev.00-01/

This latest version looks good.