Review request: 8024547: MaxMetaspaceSize should limit the committed memory used by the metaspaces

[Bengt Rutisson]

Hi Stefan,

I think think this looks really good.

Below are a couple of minor nits. Feel free to address them or ignore 
them. I'm fine with you pushing the existing changeset.



I don't think "special" is a very good name for signaling that we have 
committed all large pages up front. But since that is the name used in 
other code I find it a little confusing to suddenly call it 
"pre_committed" in metaspace.cpp. However, "pre_committed" is a better 
name, so I'm ok with keeping is_pre_committed(). But in that case we 
should probably use it instead of _rs.special() in 
VirtualSpaceNode::initialize().

  size_t pre_committed_size = _rs.special() ? _rs.size() : 0;


In MetaspaceGC::delta_capacity_until_GC(). Why do we pick min_delta in 
this case?

   if (delta <= min_delta) {
     delta = min_delta;
   }

Why not expand with max_delta for the same reason as a slightly larger 
delta?

   if (delta <= max_delta) {
     // Don't want to hit the high water mark on the next
     // allocation so make the delta greater than just enough
     // for this allocation.
     delta = max_delta;
   }



Why did you move Metadebug::deallocate_chunk_a_lot(this, 
grow_chunks_by_words); out into it's own if section?


   if (next != NULL) {
     Metadebug::deallocate_chunk_a_lot(this, grow_chunks_by_words);
   }

   MetaWord* mem = NULL;

   // If a chunk was available, add it to the in-use chunk list
   // and do an allocation from it.
   if (next != NULL) {
     // Add to this manager's list of chunks in use.
     add_chunk(next, false);
     mem = next->allocate(word_size);
   }

   return mem;

Thanks,
Bengt



On 10/8/13 8:21 AM, Stefan Karlsson wrote:
> Please, review this patch to limit the committed Metaspace memory 
> against the MaxMetaspaceSize flag.
>
> http://cr.openjdk.java.net/~stefank/8024547/webrev.00
>
> 8024547: MaxMetaspaceSize should limit the committed memory used by 
> the metaspaces
> Reviewed-by: TBD1, TBD2
>
> To simplify the code, the patch is strict about the alignments used to 
> commit and reserve memory in the metaspaces. The Metaspace 
> VirtualSpaces always have addresses and sizes that are aligned against 
> Metaspace::reserve_alignment(). The reserve alignment is 
> os::vm_allocation_granularity() if small pages are used or 
> os::large_page_size() if large pages are used in the Metaspace. The 
> memory is always committed in regions that are size aligned against 
> Metaspace::commit_alignment(). The commit alignment is os:page_size() 
> if small pages are used or os::large_page_size() if late pages are 
> used in the Metaspace. The user can specify the flag 
> -XX:+UseLargePagesInMetaspace to turn on large pages in the metaspaces.
>
> The flag initialization was moved out of the CollectorPolicy class. 
> The Metaspace specific flags have been changed to be commit/reserve 
> aligned instead of using heap specific alignments.
>
> The output from PrintHeapAtGC has been changed. The redundant "data 
> space" section has been removed. All of used, capacity, committed and 
> reserved are printed. Example:
>  Metaspace       used 7644K, capacity 7700K, committed 7808K, reserved 
> 1056768K
>   class space    used 804K, capacity 822K, committed 896K, reserved 
> 1048576K
>
> Thanks go out to the engineers who have helped out with this bug: 
> Erik, Mikael and Bengt for helping out with parts of the code and 
> testing. Bengt, Coleen, Jesper and Jon for pre-reviewing the changes 
> and providing early feedback.
>
> thanks,
> StefanK