RFR(S) 8038405: Clean up some virtual fucntions in Space class hierarchy

[Mikael Gerdin]

Jon,

On Thursday 27 March 2014 16.05.27 Jon Masamitsu wrote:
> On 3/27/2014 8:18 AM, Mikael Gerdin wrote:
> > Hi,
> > 
> > On Wednesday 26 March 2014 17.22.03 Mikael Gerdin wrote:
> >> Hi,
> >> 
> >> I'm doing some cleanups in the "Space" classes to simplify the code for
> >> the
> >> G1 class unloading work[1].
> >> This change moves around some virtual functions in Space to more (in my
> >> mind) sane places:
> >> 
> >> * Space::is_in is implemented the same by all Space subclasses, move it
> >> to
> >> Space and make it non-virtual.
> >> * Space::used_region has a bogus implementation that is never useful,
> >> remove it.
> 
> Minor point but used_region() did do as the comment says.   It just
> most often returned a region that include more than just the allocated
> objects, and not a proper subregion.   Could you change the comment to
> 
>   136   // Returns a subregion of the space
> containingonlytheallocatedobjects in 137   // the space.
> 
> if that is how it is used in all the other spaces.

That's a good point.
I've updated the comment as per your suggestion.

> 
> Rest looks good.

Thanks
/Mikael

> 
> Reviewed.
> 
> Jon
> 
> >> * Consolidate more code related to saved_marks in the Space base class
> > 
> > When making set_saved_mark non-virtual G1OffsetTableContigSpace's override
> > of set_saved_mark became a shadowing function instead. The code still
> > works fine since all calls to G1OffsetTableContigSpace::set_saved_mark
> > are on pointers to HeapRegion and not the base class.
> > 
> > To clarify that G1 uses the "saved_mark" functionality for a completely
> > different purpose I have renamed the G1 function to
> > record_top_and_timestamp:
> > 
> > Incremental webrev:
> > http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0.to.1 Webrev:
> > http://cr.openjdk.java.net/~mgerdin/8038405/webrev.1
> > 
> > /Mikael
> > 
> >> * Make CompactibleSpace::reset_after_compaction a pure virtual.
> >> * Give Space::minimum_free_block_size a default implementation since only
> >> CMS needs to override it.
> >> 
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8038405
> >> Webrev: http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0
> >> 
> >> /Mikael
> >> 
> >> [1] http://openjdk.java.net/jeps/156