RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Apr 3 17:45:34 UTC 2017
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.
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