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

[Thomas Stüfe]

Hi Mikael,

On Mon, Apr 3, 2017 at 10:38 AM, Mikael Gerdin <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?


> 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>> 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-81705
>> 20-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webrev.00/webrev/
>>     <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>> 520-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/2
>> 016-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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>