Move postGC out of CrossThreadPersistentRegion::LockScope

Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the CrossThreadPersistentRegion::LockScope. This should be okay as the CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of CrossThreadPersistentNodes during marking.

BUG=684856, 685624, 685624
Review-Url: https://codereview.chromium.org/2686533003
Cr-Commit-Position: refs/heads/master@{#450251}
Committed: https://chromium.googlesource.com/chromium/src/+/52b19c8f68c222c08944af937a01f3ff7e46705c

[keishi, 2017-02-08 06:43:50]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 06:44:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 08:40:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 08:40:26]

Dry run: This issue passed the CQ dry run.

[keishi, 2017-02-09 02:52:35]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:53:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[keishi, 2017-02-09 04:11:56]

Description was changed from

==========
Move postGC

BUG=
==========

to

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=
==========

[keishi, 2017-02-09 04:20:14]

Description was changed from

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=
==========

to

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=684856
==========

[keishi, 2017-02-09 04:21:14]

Description was changed from

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=684856
==========

to

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=684856,685624,685624
==========

[keishi, 2017-02-09 04:34:20]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2017-02-09 04:34:21]

I tried removing the lock from the AudioNode prefinalizer, but I think the way
it works, it is impossible.

We definitely need to hold the lock while marking but, I think we can move the
entire postGC step out of the CrossThreadPersistentRegion::LockScope.

[haraken, 2017-02-09 04:47:51]

On 2017/02/09 04:34:21, keishi wrote:
> I tried removing the lock from the AudioNode prefinalizer, but I think the way
> it works, it is impossible.
> 
> We definitely need to hold the lock while marking but, I think we can move the
> entire postGC step out of the CrossThreadPersistentRegion::LockScope.

Maybe you're correct.

However, for safety (for making the mental model simpler), I want to keep
threadLocalWeakProcessing() in the lock scope.

Can you split postGC() into the following two parts?

- postGC(): Calls prepareForSweep and threadLocalWeakProcessing
- preSweep(): Calls invokePreFinalizers, eagerSweep etc

And hold the lock during postGC() but not during preSweep().

> This should be okay as the CrossThreadPersistentRegion::LockScope only exists
to avoid add/deletetion of CrossThreadPersistentNodes during marking.

Remember that the lock scope is also needed to make GCs on two threads mutually
exclusive.

[commit-bot: I haz the power, 2017-02-09 05:39:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 05:40:00]

Dry run: This issue passed the CQ dry run.

[sof, 2017-02-09 06:46:01]

On 2017/02/09 04:47:51, haraken wrote:
> On 2017/02/09 04:34:21, keishi wrote:
> > I tried removing the lock from the AudioNode prefinalizer, but I think the
way
> > it works, it is impossible.
> > 
> > We definitely need to hold the lock while marking but, I think we can move
the
> > entire postGC step out of the CrossThreadPersistentRegion::LockScope.
> 
> Maybe you're correct.
> 
> However, for safety (for making the mental model simpler), I want to keep
> threadLocalWeakProcessing() in the lock scope.
> 
> Can you split postGC() into the following two parts?
> 
> - postGC(): Calls prepareForSweep and threadLocalWeakProcessing
> - preSweep(): Calls invokePreFinalizers, eagerSweep etc
> 
> And hold the lock during postGC() but not during preSweep().
> 
> > This should be okay as the CrossThreadPersistentRegion::LockScope only
exists
> to avoid add/deletetion of CrossThreadPersistentNodes during marking.
> 
> Remember that the lock scope is also needed to make GCs on two threads
mutually
> exclusive.

I'd like to understand why & where having that exclusion matters.

But given this and https://codereview.chromium.org/2660463003/ , I wonder if
there should be a separate CTPs-being-locked-during-a-GC lock.

[haraken, 2017-02-09 07:15:11]

On 2017/02/09 06:46:01, sof wrote:
> On 2017/02/09 04:47:51, haraken wrote:
> > On 2017/02/09 04:34:21, keishi wrote:
> > > I tried removing the lock from the AudioNode prefinalizer, but I think the
> way
> > > it works, it is impossible.
> > > 
> > > We definitely need to hold the lock while marking but, I think we can move
> the
> > > entire postGC step out of the CrossThreadPersistentRegion::LockScope.
> > 
> > Maybe you're correct.
> > 
> > However, for safety (for making the mental model simpler), I want to keep
> > threadLocalWeakProcessing() in the lock scope.
> > 
> > Can you split postGC() into the following two parts?
> > 
> > - postGC(): Calls prepareForSweep and threadLocalWeakProcessing
> > - preSweep(): Calls invokePreFinalizers, eagerSweep etc
> > 
> > And hold the lock during postGC() but not during preSweep().
> > 
> > > This should be okay as the CrossThreadPersistentRegion::LockScope only
> exists
> > to avoid add/deletetion of CrossThreadPersistentNodes during marking.
> > 
> > Remember that the lock scope is also needed to make GCs on two threads
> mutually
> > exclusive.
> 
> I'd like to understand why & where having that exclusion matters.
> 
> But given this and https://codereview.chromium.org/2660463003/ , I wonder if
> there should be a separate CTPs-being-locked-during-a-GC lock.

I'm confused. CrossThreadPersistentRegion::m_mutex is a recursive mutex. Why can
it cause a dead lock?

[sof, 2017-02-09 07:25:16]

On 2017/02/09 07:15:11, haraken wrote:
> On 2017/02/09 06:46:01, sof wrote:
> > On 2017/02/09 04:47:51, haraken wrote:
> > > On 2017/02/09 04:34:21, keishi wrote:
> > > > I tried removing the lock from the AudioNode prefinalizer, but I think
the
> > way
> > > > it works, it is impossible.
> > > > 
> > > > We definitely need to hold the lock while marking but, I think we can
move
> > the
> > > > entire postGC step out of the CrossThreadPersistentRegion::LockScope.
> > > 
> > > Maybe you're correct.
> > > 
> > > However, for safety (for making the mental model simpler), I want to keep
> > > threadLocalWeakProcessing() in the lock scope.
> > > 
> > > Can you split postGC() into the following two parts?
> > > 
> > > - postGC(): Calls prepareForSweep and threadLocalWeakProcessing
> > > - preSweep(): Calls invokePreFinalizers, eagerSweep etc
> > > 
> > > And hold the lock during postGC() but not during preSweep().
> > > 
> > > > This should be okay as the CrossThreadPersistentRegion::LockScope only
> > exists
> > > to avoid add/deletetion of CrossThreadPersistentNodes during marking.
> > > 
> > > Remember that the lock scope is also needed to make GCs on two threads
> > mutually
> > > exclusive.
> > 
> > I'd like to understand why & where having that exclusion matters.
> > 
> > But given this and https://codereview.chromium.org/2660463003/ , I wonder if
> > there should be a separate CTPs-being-locked-during-a-GC lock.
> 
> I'm confused. CrossThreadPersistentRegion::m_mutex is a recursive mutex. Why
can
> it cause a dead lock?

It can't, this wasn't a question on safety. I was just wondering if we can avoid
contention on lock-taking between threads assigning CTPs -- those thread
operations will have to be locked out while a GC runs, but should ideally
otherwise be allowed to concurrently happen. But upon further thought, there
doesn't seem to be a way to improve on having that current global lock.

Back the first issue above, why is this lock also needed to enforce exclusion on
GCs between threads?

[haraken, 2017-02-09 07:35:28]

On 2017/02/09 07:25:16, sof wrote:
> On 2017/02/09 07:15:11, haraken wrote:
> > On 2017/02/09 06:46:01, sof wrote:
> > > On 2017/02/09 04:47:51, haraken wrote:
> > > > On 2017/02/09 04:34:21, keishi wrote:
> > > > > I tried removing the lock from the AudioNode prefinalizer, but I think
> the
> > > way
> > > > > it works, it is impossible.
> > > > > 
> > > > > We definitely need to hold the lock while marking but, I think we can
> move
> > > the
> > > > > entire postGC step out of the CrossThreadPersistentRegion::LockScope.
> > > > 
> > > > Maybe you're correct.
> > > > 
> > > > However, for safety (for making the mental model simpler), I want to
keep
> > > > threadLocalWeakProcessing() in the lock scope.
> > > > 
> > > > Can you split postGC() into the following two parts?
> > > > 
> > > > - postGC(): Calls prepareForSweep and threadLocalWeakProcessing
> > > > - preSweep(): Calls invokePreFinalizers, eagerSweep etc
> > > > 
> > > > And hold the lock during postGC() but not during preSweep().
> > > > 
> > > > > This should be okay as the CrossThreadPersistentRegion::LockScope only
> > > exists
> > > > to avoid add/deletetion of CrossThreadPersistentNodes during marking.
> > > > 
> > > > Remember that the lock scope is also needed to make GCs on two threads
> > > mutually
> > > > exclusive.
> > > 
> > > I'd like to understand why & where having that exclusion matters.
> > > 
> > > But given this and https://codereview.chromium.org/2660463003/ , I wonder
if
> > > there should be a separate CTPs-being-locked-during-a-GC lock.
> > 
> > I'm confused. CrossThreadPersistentRegion::m_mutex is a recursive mutex. Why
> can
> > it cause a dead lock?
> 
> It can't, this wasn't a question on safety. I was just wondering if we can
avoid
> contention on lock-taking between threads assigning CTPs -- those thread
> operations will have to be locked out while a GC runs, but should ideally
> otherwise be allowed to concurrently happen. But upon further thought, there
> doesn't seem to be a way to improve on having that current global lock.
> 
> Back the first issue above, why is this lock also needed to enforce exclusion
on
> GCs between threads?

Yeah, agreed. Now that the heap is completely isolated per thread, there will be
no reason to make GCs between threads exclusive.

[keishi, 2017-02-10 12:11:40]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 12:11:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 12:13:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 12:13:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[keishi, 2017-02-13 02:37:46]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 02:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 03:34:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 03:34:16]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[keishi, 2017-02-13 04:17:02]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 04:17:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 04:43:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 04:43:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[keishi, 2017-02-13 08:20:35]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 08:20:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 08:22:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 08:22:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[keishi, 2017-02-13 08:32:19]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 08:32:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 10:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 10:04:44]

Dry run: This issue passed the CQ dry run.

[keishi, 2017-02-13 12:24:36]

On 2017/02/09 04:47:51, haraken wrote:
> On 2017/02/09 04:34:21, keishi wrote:
> > I tried removing the lock from the AudioNode prefinalizer, but I think the
way
> > it works, it is impossible.
> > 
> > We definitely need to hold the lock while marking but, I think we can move
the
> > entire postGC step out of the CrossThreadPersistentRegion::LockScope.
> 
> Maybe you're correct.
> 
> However, for safety (for making the mental model simpler), I want to keep
> threadLocalWeakProcessing() in the lock scope.
> 
> Can you split postGC() into the following two parts?
> 
> - postGC(): Calls prepareForSweep and threadLocalWeakProcessing
> - preSweep(): Calls invokePreFinalizers, eagerSweep etc
> 
> And hold the lock during postGC() but not during preSweep().
Done. PTAL

[haraken, 2017-02-13 16:56:24]

LGTM

https://codereview.chromium.org/2686533003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2686533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:230: //   
pre-finalization, eager sweeping and heap compaction.

Update this comment.

[keishi, 2017-02-14 01:18:21]

https://codereview.chromium.org/2686533003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2686533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:230: //   
pre-finalization, eager sweeping and heap compaction.
On 2017/02/13 16:56:24, haraken wrote:
> 
> Update this comment.

Done.

[keishi, 2017-02-14 01:25:57]

The CQ bit was checked by keishi@chromium.org

[keishi, 2017-02-14 01:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2686533003/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2017-02-14 01:26:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 04:47:50]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487035557359240,
"parent_rev": "4c6747c1032af89b54e32d268439add818cbe37b", "commit_rev":
"52b19c8f68c222c08944af937a01f3ff7e46705c"}

[commit-bot: I haz the power, 2017-02-14 04:48:26]

Description was changed from

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=684856,685624,685624
==========

to

==========
Move postGC out of CrossThreadPersistentRegion::LockScope

Some finalizers/prefinalizers acquire locks, causing a dead lock when
CrossThreadPersistent tries to acquire the CrossThreadPersistentRegion lock.

This CL moves the postGC step outside of the
CrossThreadPersistentRegion::LockScope. This should be okay as the
CrossThreadPersistentRegion::LockScope only exists to avoid add/deletetion of
CrossThreadPersistentNodes during marking.

BUG=684856,685624,685624

Review-Url: https://codereview.chromium.org/2686533003
Cr-Commit-Position: refs/heads/master@{#450251}
Committed:
https://chromium.googlesource.com/chromium/src/+/52b19c8f68c222c08944af937a01...
==========

[commit-bot: I haz the power, 2017-02-14 04:48:28]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/52b19c8f68c222c08944af937a01...