RFR: 8190711: Assert in G1MMUTracker due to concurrent modification

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8190711: Assert in G1MMUTracker due to concurrent modification

Stefan Johansson
Hi all,

Please review this fix for:
https://bugs.openjdk.java.net/browse/JDK-8190711

Webrev:
http://cr.openjdk.java.net/~sjohanss/8190711/00/

Summary:
The G1MMUTracker is used both by the concurrent gc threads and during
STW by the VM thread. Up until this change we have had modifications
done to the internal data structure from both sides. To synchronize we
have used the SuspendibleThreadSet but this doesn't stop two different
concurrent threads from modifying the data at the same time. We need to
prevent this.

The reason to update the data structure (remove entires) in the
concurrent phase is a possible performance improvement due to having
fewer entries to iterate when calling when_sec(). Since there can never
be more than 64 entries, the gain, if any, is very small. We still
remove expired entries when adding new ones, done by add_pause() during
STW. To avoid having more than one thread doing modification of the list
this change removes the call to remove_expired_entries() in when_sec().
Having the SuspendibleThreadSet will ensure the the concurrent threads
won't access the data while the VM thread is updating it.

Testing:
* Tier 1-2

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8190711: Assert in G1MMUTracker due to concurrent modification

Thomas Schatzl
Hi,

On Wed, 2017-11-08 at 11:47 +0100, Stefan Johansson wrote:

> Hi all,
>
> Please review this fix for:
> https://bugs.openjdk.java.net/browse/JDK-8190711
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8190711/00/
>
> Summary:
> The G1MMUTracker is used both by the concurrent gc threads and
> during STW by the VM thread. Up until this change we have had
> modifications done to the internal data structure from both sides. To
> synchronize we have used the SuspendibleThreadSet but this doesn't
> stop two different concurrent threads from modifying the data at the
> same time. We need to prevent this.
>
> The reason to update the data structure (remove entires) in the 
> concurrent phase is a possible performance improvement due to having 
> fewer entries to iterate when calling when_sec(). Since there can
> never be more than 64 entries, the gain, if any, is very small. We
> still remove expired entries when adding new ones, done by
> add_pause() during STW. To avoid having more than one thread doing
> modification of the list this change removes the call to
> remove_expired_entries() in when_sec(). 
> Having the SuspendibleThreadSet will ensure the the concurrent
> threads won't access the data while the VM thread is updating it.

  looks good.

There does not seem to be any difference in the number of entries we
can have in that list, as add_pause() is the only one adding entries
anyway.
As you note we might only get some more expired entries to work
through.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8190711: Assert in G1MMUTracker due to concurrent modification

Robbin Ehn
In reply to this post by Stefan Johansson
Thanks for fixing this fast!

 From what I can tell, looks good!

/Robbin

On 11/08/2017 11:47 AM, Stefan Johansson wrote:

> Hi all,
>
> Please review this fix for:
> https://bugs.openjdk.java.net/browse/JDK-8190711
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8190711/00/
>
> Summary:
> The G1MMUTracker is used both by the concurrent gc threads and during STW by the
> VM thread. Up until this change we have had modifications done to the internal
> data structure from both sides. To synchronize we have used the
> SuspendibleThreadSet but this doesn't stop two different concurrent threads from
> modifying the data at the same time. We need to prevent this.
>
> The reason to update the data structure (remove entires) in the concurrent phase
> is a possible performance improvement due to having fewer entries to iterate
> when calling when_sec(). Since there can never be more than 64 entries, the
> gain, if any, is very small. We still remove expired entries when adding new
> ones, done by add_pause() during STW. To avoid having more than one thread doing
> modification of the list this change removes the call to
> remove_expired_entries() in when_sec(). Having the SuspendibleThreadSet will
> ensure the the concurrent threads won't access the data while the VM thread is
> updating it.
>
> Testing:
> * Tier 1-2
>
> Thanks,
> Stefan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8190711: Assert in G1MMUTracker due to concurrent modification

sangheon.kim@oracle.com
In reply to this post by Stefan Johansson
Hi Stefan,

Thank you for fixing this bug!

On 11/08/2017 02:47 AM, Stefan Johansson wrote:

> Hi all,
>
> Please review this fix for:
> https://bugs.openjdk.java.net/browse/JDK-8190711
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8190711/00/
>
> Summary:
> The G1MMUTracker is used both by the concurrent gc threads and during
> STW by the VM thread. Up until this change we have had modifications
> done to the internal data structure from both sides. To synchronize we
> have used the SuspendibleThreadSet but this doesn't stop two different
> concurrent threads from modifying the data at the same time. We need
> to prevent this.
>
> The reason to update the data structure (remove entires) in the
> concurrent phase is a possible performance improvement due to having
> fewer entries to iterate when calling when_sec(). Since there can
> never be more than 64 entries, the gain, if any, is very small. We
> still remove expired entries when adding new ones, done by add_pause()
> during STW. To avoid having more than one thread doing modification of
> the list this change removes the call to remove_expired_entries() in
> when_sec(). Having the SuspendibleThreadSet will ensure the the
> concurrent threads won't access the data while the VM thread is
> updating it.
Looks good.

Thanks,
Sangheon


>
> Testing:
> * Tier 1-2
>
> Thanks,
> Stefan