RFR (M): 8189737: Make HeapRegion not derive from Space

[Stefan Johansson]

Hi Thomas,

On 2019-11-06 10:22, Thomas Schatzl wrote:
> Hi Stefan,
> 
>    thanks for your review.
> 
> On 05.11.19 12:35, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2019-11-04 17:40, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On 01.11.19 00:20, Thomas Schatzl wrote:
>>>> Hi Kim,
>>>>
>>>>    thanks for your review.
>>>>
>>>> On Thu, 2019-10-31 at 18:12 -0400, Kim Barrett wrote:
>>>>>> On Oct 31, 2019, at 9:43 AM, Thomas Schatzl <
>>>>>> thomas.schatzl at oracle.com> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>>   can I get reviews for this refactoring that removes the
>>>>>> inheritance of HeapRegion from Space?
>>>>>>
>>>>>>
>>>> [...]
>>>>>> CR:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8189737
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~tschatzl/8189737/webrev/
>>
>> I think this looks really good in general, so nice to get rid of this 
>> inheritance.
>>
>> I have one question/comment though. I took a look at the HeapRegion 
>> implementation in the SA and there it still extends CompactibleSpace 
>> (which extends Space), I think we should remove this and add _bottom 
>> and _top for HeapRegion in vmStructs_g1.hpp. This also has the effect 
>> that the PrintRegionClosure needs to be updated to not depend on space 
>> either. It is only used by G1 so I think it should simply be moved 
>> from shared to g1 and changed to not depend on space.
> 
> Good catch. Changed in
> 
> http://cr.openjdk.java.net/~tschatzl/8189737/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8189737/webrev.1 (full)

Thanks for fixing this, looks good. I don't think we need to add 
_compaction_top, from what I can see it is not used. I don't need a new 
webrev for this if I'm right.

Thanks,
Stefan

> 
> I ran hs-tier6 (containing G1 SA tests) and jtreg/serviceability/sa 
> tests with that with no issue.
> 
> Thanks,
>    Thomas