RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
Thomas Stüfe
thomas.stuefe at gmail.com
Mon Apr 3 14:16:32 UTC 2017
Hi Mikael,
thanks for the review. New Version:
http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.01/webrev/
Changes to before:
- reworked ChunkManager::dec_free_chunks_total() and
ChunkManager::inc_free_chunks_total() to
ChunkManager::account_removed_chunk() and
ChunkManager::account_added_chunk(), as discussed. Methods now take a const
pointer to the Metachunk added/removed, after it has been added/removed.
- added assert_lock_strong as suggested. Note that this made it necessary
to move the methods out of the class body to be able to access
SpaceManager::expand_lock(), which is defined after ChunkManager.
Kind Regards, Thomas
On Mon, Apr 3, 2017 at 1:59 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:
> Hi Thomas,
>
> On 2017-04-03 11:22, Thomas Stüfe wrote:
>
>> Hi Mikael,
>>
>> On Mon, Apr 3, 2017 at 10:38 AM, Mikael Gerdin <mikael.gerdin at oracle.com
>> <mailto:mikael.gerdin at oracle.com>> wrote:
>>
>> 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?
>>
>
> Since all callers just pass in chunk->word_size() I think that makes sense.
>
> /Mikael
>
>
>>
>> To document that the inc and dec operations are properly
>> synchronized maybe we should add
>> assert_lock_strong(SpaceManager::expand_lock());
>> to inc and dec to show that?
>>
>>
>> Yes, definitely.
>>
>> ...Thomas
>>
>>
>> /Mikael
>>
>>
>> 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>
>> <mailto:thomas.stuefe at gmail.com
>>
>> <mailto:thomas.stuefe at gmail.com>>> wrote:
>>
>> Hi,
>>
>> https://bugs.openjdk.java.net/browse/JDK-8170520
>> <https://bugs.openjdk.java.net/browse/JDK-8170520>
>> <https://bugs.openjdk.java.net/browse/JDK-8170520
>> <https://bugs.openjdk.java.net/browse/JDK-8170520>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-81705
>> 20-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-
>> webrev.00/webrev/>
>>
>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-
>> webrev.00/webrev/
>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-
>> webrev.00/webrev/>>
>>
>> may I please get a review of another small cleanup change to
>> the
>> metaspace. Compared with the last one (JDK-8170933), this one
>> is
>> smaller.
>>
>> I posted this originally for jdk 9 last november, and it got
>> reviewed already:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>> 016-November/021946.html
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
>> 2016-November/021946.html>
>>
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
>> 2016-November/021946.html
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
>> 2016-November/021946.html>>.
>>
>> 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
>> JDK-8170933.
>>
>> 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 updates.
>>
>> 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
>> ChunkManager::return_single_chunk().
>>
>> 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,
>>
>> Thomas
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-dev
mailing list