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

[Mikael Gerdin]

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?

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?

/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>> wrote:
>
>     Hi,
>
>     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/>
>
>     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>.
>     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
>
>
>
>
>
>
>
>
>