RFR: 8251158: Implementation of JEP 387: Elastic Metaspace [v11]

[Thomas Stuefe]

On Tue, 6 Oct 2020 07:00:51 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/memory/metaspace/metaspaceArena.cpp line 81:
>> 
>>> 79:            METACHUNK_FULL_FORMAT_ARGS(c));
>>> 80:   }
>>> 81: }
>> 
>> Have you thought about splitting the chunk if possible and returning the free chunk to the ChunkManager? If you return
>> it to the free list of this class loader then only this loader can make use of it. If you return it to the CM then
>> every loader can make use of it and, potentially, it can even be uncommitted.
>
> Yes, but in practice, the benefit would be slim:
> - it is very rare that you retire a chunk which is not even halfway used. For that to happen a metaspace allocation must
>   be requested larger than half of the current chunk. Does not happen that often.
> - This only affects the committed portion of the chunk. So, for larger chunks - where this could hurt - the waste (waste
>   in the sense as "unnecessarily earmarked for one loader only") mostly consists of address space only, not committed
>   space.
> 
> I'll still consider it. Albert Yang remarked something similar in an earlier review:
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041357.html
> Thing is, returning a chunk to the ChunkManager would require drawing the central context lock. Right now,
> salvage_chunk() does only need the CLD-local lock. But then, if it is rare as I wrote initially, it may not matter.
> Lets file this for a possible future RFE.

Okay, I did some statistics and salvaging chunks which are not at least half-way used is super rare. I opt in this case
for just leaving the code as it is since splitting the salvaged chunk would mean complicating the code, apart from
drawing the central metaspace context lock.

>> src/hotspot/share/memory/metaspace/counters.hpp line 108:
>> 
>>> 106:     assert(old >= 1,
>>> 107:         "underflow (" UINT64_FORMAT "-1)", (uint64_t)old);
>>> 108: #endif
>> 
>> The assertion is incorrect because it is not atomic wrt the actual update. I commented on this before. It should be
>> fairly easy to write a multi-thread gtest that produces false positives of the assertion (if it is easy to write a
>> multi-threaded gtest). The person affected by a false positive crash will be not too amused. I suggest to fix or remove
>> the assertion.
>
> Okay, I feel stupid now but I do not see it.
> 
> I see the non-atomicity, but not how it could cause a false positive assert here. I see that I could miss an assert,
> but not the other way around.

I will remove the asserts. These classes are scheduled to become general utility classes anyway in a followup RFE, so
they will be revisited soon.

-------------

PR: https://git.openjdk.java.net/jdk/pull/336