RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Apr 3 17:03:08 UTC 2017
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?
I can sponsor your change.
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