RFR(xs): 8170520: Make Metaspace ChunkManager counters non-atomic

[Mikael Gerdin]

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-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/~stuefe/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/>>
>
>             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>>.
>
>             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
>
>
>
>
>
>
>
>
>
>