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

[Mikael Gerdin]

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 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/

/Mikael

> 
> thanks,
> StefanK
> 
> > /Mikael