RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
Thomas Stüfe
thomas.stuefe at gmail.com
Mon Apr 3 17:37:59 UTC 2017
Hi Coleen,
On Mon, Apr 3, 2017 at 7:03 PM, <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/
Only the names of the functions changed as requested, the rest is unchanged.
I can sponsor your change.
>
>
Thank you, 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/
>
> 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-webr
>>> ev.00/webrev/
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.00/webrev/>
>>>
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.00/webrev/
>>> <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.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