RFR(S) 8015255: NPG: Don't waste fragment at the end of a VirtualSpaceNode before retiring it.

[Mikael Gerdin]

Stefan,

On Thursday 17 October 2013 17.31.34 Stefan Karlsson wrote:
> On 10/17/13 5:09 PM, Mikael Gerdin wrote:
> > Stefan,
> > 
> > Thanks for looking at this, I've replied to your comments inline.
> > 
> > On Thursday 17 October 2013 16.15.03 Stefan Karlsson wrote:
> >> On 2013-10-17 14:56, Mikael Gerdin wrote:
> >>> Hi all,
> >>> 
> >>> Please review this change to fix needless memory waste in Metaspace.
> >>> 
> >>> Description of problem:
> >>> When receiving an allocation request which cannot be satisfied with the
> >>> current VirtualSpaceNode the remaining committed memory in the current
> >>> node is wasted when a new node is created and the allocation is
> >>> satisfied
> >>> from the new node.
> >>> 
> >>> Suggested fix:
> >>> Before creating a new VirtualSpaceNode, "retire" the current node by
> >>> allocating chunks of decreasing size from the committed memory present
> >>> in
> >>> the current node.
> >>> To ensure that allocating chunks at the end of a node uses all committed
> >>> memory I've increased the size alignment for humongous chunks to the
> >>> smallest chunk size. I don't foresee this increasing memory waste since
> >>> previously the memory difference between 2-word-aligned sizes and
> >>> smallest chunk-aligned would go unused since no chunk would fit there.
> >>> 
> >>> Testing:
> >>> Added new unit tests for VirtualSpaceNode and the retire functionality.
> >>> JPRT run (including the new unit tests)
> >>> 
> >>> Webrev: http://cr.openjdk.java.net/~mgerdin/8015255/webrev.0/
> >>> Buglink: https://bugs.openjdk.java.net/browse/JDK-8015255
> >> 
> >> Very nice.
> >> 
> >> Some comments:
> >> 
> >> Would it make more sense to move the call to
> >> retire_current_virtual_space() into create_new_virtual_space() or
> >> link_vs()?> 
> > If I do that I would need to first check if there is a current virtual
> > space, since create_new_virtual_space() (and and its callee link_vs())
> > are called when a VirtualSpaceList is first created.
> > I'd rather keep it in the current place in the code, where I don't need to
> > wrap it with if (current_virtual_space() != NULL) {}
> 
> I think it makes more sense to retire the current virtual space when
> it's actually being replaced by a new virtual space.
> 
> We already have the needed check in link_vs:
> void VirtualSpaceList::link_vs(VirtualSpaceNode* new_entry) {
>    if (virtual_space_list() == NULL) {
>        set_virtual_space_list(new_entry);
>    }

I see your point, but I feel like the call would be lost in the manual linked 
list management in that if-else-statement.

> 
> But if you don't want to do it I'm not going to fight for it.

I'd prefer to keep it as-is.

> 
> >> I think some punctation is missing between the two first lines:
> >> +  // If an allocation doesn't fit in the current node a new node is
> >> created +  // allocate chunks out of the remaining committed space in
> >> this node + // to avoid wasting that memory.
> > 
> > Fixed
> > 
> >> I think you have an extra "the" in this sentence:
> >> +  // This always adds up because the all the chunk sizes are multiples
> >> of
> >> +  // the smallest chunk size.
> > 
> > Fixed
> > 
> >> The other usages of HumongousChunkGranularity have to be changed to
> >> smallest_chunk_size().
> > 
> > Fixed, and removed HumongousChunkGranularity
> > 
> >> I had a hard time understanding this test:
> >> +    { // 4 pages of VSN is committed, some is used by chunks
> >> 
> >> could you change this line:
> >> +      const size_t page_chunks = MIN2(4 * (size_t)os::vm_page_size() /
> >> BytesPerWord, vsn_test_size_words);
> >> 
> >> to:
> >>          const size_t page_chunks = 4 * (size_t)os::vm_page_size() /
> >> 
> >> BytesPerWord;
> >> 
> >> and add an assert that 4 * (size_t)os::vm_page_size() is less than a
> >> MediumChunk (or less than MediumChunk + SmallChunk + SpecializedChunk)?
> > 
> > Good point, I changed it as you suggested. I added an assert that
> > (page_chunks < MediumChunk)
> > 
> > Incremental webrev:
> > http://cr.openjdk.java.net/~mgerdin/8015255/webrev.0-to-1/ New webrev:
> > http://cr.openjdk.java.net/~mgerdin/8015255/webrev.1/
> 
> Great! Looks good.

Thanks for reviewing!
/Mikael

> 
> thanks,
> StefanK
> 
> > /Mikael
> > 
> >> thanks,
> >> StefanK
> >> 
> >>> /Mikael