Quantcast

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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi,

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/

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. 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Ping... nobody?

Kind Regards, Thomas

On Tue, Mar 21, 2017 at 9:59 AM, Thomas Stüfe <[hidden email]>
wrote:

> Hi,
>
> 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/
>
> 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. 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
>
>
>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]
> <mailto:[hidden email]>> 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
>
>
>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Mikael,

On Mon, Apr 3, 2017 at 10:38 AM, Mikael Gerdin <[hidden email]>
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]
> <mailto:[hidden email]>> 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
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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
>
>
>
>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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/

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 <[hidden email]>
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>         <[hidden email] <mailto:[hidden email]>
>>         <mailto:[hidden email]
>>
>>         <mailto:[hidden email]>>> 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-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/>
>>
>>         <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>> 520-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>
>>
>>         <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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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
> <[hidden email] <mailto:[hidden email]>> 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
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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
>                 <[hidden email]
>         <mailto:[hidden email]>
>         <mailto:[hidden email] <mailto:[hidden email]>>
>                 <mailto:[hidden email]
>         <mailto:[hidden email]>
>
>                 <mailto:[hidden email]
>         <mailto:[hidden email]>>>> 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
>
>
>
>
>
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Coleen,


On Mon, Apr 3, 2017 at 7:03 PM, <[hidden email]> 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/

Only the names of the functions changed as requested, the rest is unchanged.

I can sponsor your change.
>
>
Thank you, 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/
>
> 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 <[hidden email]>
> 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 <[hidden email]
>>> <mailto:[hidden email]>> 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
>>>         <[hidden email] <mailto:[hidden email]>
>>>         <mailto:[hidden email]
>>>
>>>         <mailto:[hidden email]>>> 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-81705
>>> 20-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-webr
>>> ev.00/webrev/
>>>         <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.00/webrev/>
>>>
>>>         <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.00/webrev/
>>>         <http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-2-8170
>>> 520-Make-Metaspace-ChunkManager-Counters-NonAtomic/jdk10-web
>>> rev.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>
>>>
>>>         <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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 4/3/17 1:37 PM, Thomas Stüfe wrote:

> Hi Coleen,
>
>
> On Mon, Apr 3, 2017 at 7:03 PM, <[hidden email]
> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>> 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
>>             <[hidden email] <mailto:[hidden email]>
>>             <mailto:[hidden email]
>>             <mailto:[hidden email]>>> 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
>>                     <[hidden email]
>>             <mailto:[hidden email]>
>>             <mailto:[hidden email]
>>             <mailto:[hidden email]>>
>>                     <mailto:[hidden email]
>>             <mailto:[hidden email]>
>>
>>                     <mailto:[hidden email]
>>             <mailto:[hidden email]>>>> 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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mikael Gerdin


On 2017-04-03 19:45, [hidden email] wrote:

>
>
> On 4/3/17 1:37 PM, Thomas Stüfe wrote:
>> Hi Coleen,
>>
>>
>> On Mon, Apr 3, 2017 at 7:03 PM, <[hidden email]
>> <mailto:[hidden email]>> 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.

Fine by me.
/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
>>>     <[hidden email] <mailto:[hidden email]>> 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
>>>             <[hidden email] <mailto:[hidden email]>
>>>             <mailto:[hidden email]
>>>             <mailto:[hidden email]>>> 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
>>>                     <[hidden email]
>>>             <mailto:[hidden email]>
>>>             <mailto:[hidden email]
>>>             <mailto:[hidden email]>>
>>>                     <mailto:[hidden email]
>>>             <mailto:[hidden email]>
>>>
>>>                     <mailto:[hidden email]
>>>             <mailto:[hidden email]>>>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
Loading...