RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
Mikael Gerdin
mikael.gerdin at oracle.com
Tue Apr 4 13:38:23 UTC 2017
On 2017-04-03 19:45, coleen.phillimore at oracle.com wrote:
>
>
> On 4/3/17 1:37 PM, Thomas Stüfe wrote:
>> Hi Coleen,
>>
>>
>> On Mon, Apr 3, 2017 at 7:03 PM, <coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at oracle.com>> wrote:
>>
>> This looks good to me. I have a small request.
>> Can you change it to account_for_added_chunk() and
>> account_for_removed_chunk() because these names read better to me?
>>
>>
>> Certainly :)
>>
>> here you go:
>> http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.02/webrev/
>> <http://cr.openjdk.java.net/%7Estuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.02/webrev/>
>>
>> Only the names of the functions changed as requested, the rest is
>> unchanged.
>>
>> I can sponsor your change.
>>
>>
>> Thank you, Coleen!
>
> I'll wait for the final word from Mikael.
Fine by me.
/Mikael
> Thanks,
> Coleen
>
>> ..Thomas
>>
>>
>> thanks,
>> Coleen
>>
>>
>> On 4/3/17 10:16 AM, Thomas Stüfe wrote:
>>> 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/
>>> <http://cr.openjdk.java.net/%7Estuefe/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 <mailto: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>
>>> <mailto: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>>
>>> <mailto: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>>
>>>
>>> <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-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>>> <http://cr.openjdk.java.net/%7Estuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/>
>>>
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>>> <http://cr.openjdk.java.net/%7Estuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/>>
>>>
>>>
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>>> <http://cr.openjdk.java.net/%7Estuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/>
>>>
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>>> <http://cr.openjdk.java.net/%7Estuefe/webrevs/METASPACE-2-8170520-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/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
>>> <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>
>>>
>>> <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