RFR: 8036025 - Sort the freelist in order to shrink the heap

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Tue Mar 18 15:07:47 UTC 2014


Hi Thomas,

Thanks for looking at this. New webrev here:

http://cr.openjdk.java.net/~jwilhelm/8036025/webrev.4/

Comments inline.

Thomas Schatzl skrev 18/3/14 13:31:
> Hi Jesper,
>
>    thanks for updating the change with the latest state of the repo.
>
> On Tue, 2014-03-18 at 12:51 +0100, Jesper Wilhelmsson wrote:
>> Hi,
>>
>> Recent changes in the GC code lead to non-trivial merge conflicts. I have
>> updated the change and here is a new webrev:
>>
>> http://cr.openjdk.java.net/~jwilhelm/8036025/webrev.3/
>>
>> The change itself should be the same but several name changes and cleanups in
>> related code was made that caused the conflicts.
>
>    looks good. I agree with your suggestion in the other mail to postpone
> better encapsulation for later.
>
> Some minor nits:
>
> - g1CollectedHeap.cpp:575: the comment is not really required here. I do
> not see a reason to document that remove_region() call particularly.
> Also this is imo already documented by the
> FreeRegionList::remove_region().

Agreed and fixed.

> Not sure if we need the "_region" suffix in that method's name as others
> do not have it either. Maybe "remove_head_or_tail()" or just "remove",
> or just keep it, as you wish.

I wanted to call it just FreeRegionList::remove(bool) but for some reason the 
windows build failed due to HeapRegionSetBase::remove(HeapRegion*), so I changed 
the name for now.

> - g1CollectedHeap.cpp:600: the comment needs to be adapted. It talks
> about remove_head_or_null() still.

Fixed.

> - g1CollectedHeap.cpp:6419, 6479: the generally accepted guideline for
> parameter passing when it does not fit into a line is to use an extra
> line for each of the parameters.

Is there really any guideline regarding coding style that is not generally 
accepted in HotSpot? ;-)

Anyhow, I fixed it.

> - g1CollectedHeap.hpp:507: "humongous objet," -> "humongous object" :)

Didn't I fix this already?

Thanks,
/Jesper

>
> Thanks,
>    Thomas
>
>



More information about the hotspot-gc-dev mailing list