RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
thomas.stuefe at gmail.com
Mon Apr 3 09:22:55 UTC 2017
On Mon, Apr 3, 2017 at 10:38 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
> Hi Thomas,
> On 2017-04-02 13:26, Thomas Stüfe wrote:
>> Ping... nobody?
> Sorry for delaying looking at this.
> I have two questions:
> It looks like all callers of inc and dec both only ever pass "1" as
> num_chunks, should we remove the parameter instead?
You are right, makes sense.
Alternativly, one also could hand in the pointer to the Metachunk about to
be added/removed from the ChunkManager as in
void account_added_chunk(const Metachunk* c) // increment counters
void account_removed_chunk(const Metachunk* c) // decrement counters
which I would like a but more but it is only cosmetic. What do you think?
> To document that the inc and dec operations are properly synchronized
> maybe we should add
> to inc and dec to show that?
>> Kind Regards, Thomas
>> On Tue, Mar 21, 2017 at 9:59 AM, Thomas Stüfe <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>> wrote:
>> may I please get a review of another small cleanup change to the
>> metaspace. Compared with the last one (JDK-8170933), this one is
>> I posted this originally for jdk 9 last november, and it got
>> reviewed already:
>> Mikael found a small bug and by then it was too late to bring this
>> into jdk9, so I postponed the patch for jdk10.
>> Now the patch got rebased to Jdk10, and it is also quite a bit
>> simpler because it meshes well with the cleanup work done on
>> The change replaces the calls to Atomic::inc/dec for the
>> ChunkManager counters with simple +/-, because all code paths which
>> modify the ChunkManager counters are under lock protection
>> (SpaceManager::expand_lock()). This makes updating these counters
>> cheep and thus removes the need to be frugal with the number of
>> Before the patch - when a list of chunks was returned to a
>> ChunkManager in ~SpaceManager() - the increment values were updated
>> once, with just one call to ChunkManager::inc_free_chunks_total().
>> This left a time window in which the counters did not reflect
>> reality, so one had to be really careful where to place asserts to
>> check the ChunkManager state. That made modifying and playing with
>> the code error prone.
>> Since JDK-8170933, chunks are returned to the ChunkManager via one
>> common path, which always ends in
>> ChunkManager::return_single_chunk(). Because of that and because
>> updating the counters is now cheap, I moved the several invocations
>> of ChunkManager::inc_free_chunks_total() to
>> The rest of the changes is cosmetical:
>> - Moved ChunkManager::inc_free_chunks_total() up to the private
>> section of the class, because it is not called from outside anymore
>> - renamed arguments for ChunkManager::inc_free_chunks_total()
>> and ChunkManager::dec_free_chunks_total() to be clearer named, and
>> gave both of them the same arguments
>> - Added an assert to both function asserting that the
>> increment/decrement word size value should be a multiple of the
>> smallest chunk size
>> I ran gtests and jtreg tests on Linux and AIX, no issues popped up.
>> Thank you for reviewing,
More information about the hotspot-dev