Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1938313003:
Move MainThreadTaskRunner off Oilpan heap to simplify posting. (Closed)
|
Created:
4 years, 7 months ago by sof Modified:
4 years, 6 months ago CC:
chromium-reviews, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove MainThreadTaskRunner off Oilpan heap to simplify posting.
Having the Document's MainThreadTaskRunner on the Oilpan heap
is preferable for three reasons:
- Correctly accounts for the MainThreadTaskRunner::m_context
back reference, by having it be traced Member<>.
- The MainThreadTaskRunner must not perform tasks when
it (and the Document) is in the process of being swept.
By having the posted tasks keep a weak persistent reference
to MainThreadTaskRunner, the Oilpan GC will ensure that
the weak references will be cleared once MainThreadTaskRunner
has been deemed garbage.
- Similarly for the timer-initiated running of a
MainThreadTaskRunner's pending tasks. The Timer<> abstraction
takes care of not firing a timer if its owner is an
Oilpan heap object that's about to be swept.
But it is not without downsides:
- A CrossThreadWeakPersistent<> has to be created for every
task closure posted to the main thread, and copying that
persistent reference around while creating the closure,
something that is not without overhead.
- Threads not attached to Oilpan needing to post tasks to
the main thread will have to create these persistents also.
Having that happen when a GC is in progress is hard to support,
as it risks introducing and removing persistent heap references
in ways that interfere with the GC processing the heap.
The latter point is sufficient reason not to require the
allocation of CrossThreadWeakPersistent<>s when posting main
thread tasks, hence MainThreadTaskRunner is moved off the
Oilpan heap. By doing so, the benefits above that the Oilpan GC
infrastructure provided "for free" have to be taken care of
manually. C'est la vie.
R=
BUG=610477
Committed: https://crrev.com/93ddd5fff7fbe018efc1df2be3fe907103314422
Cr-Commit-Position: refs/heads/master@{#396432}
Patch Set 1 #
Total comments: 2
Patch Set 2 : experiment: trigger assert on CrossThreadPersistentRegion access during GC #Patch Set 3 : experiment: trigger assert on CrossThreadPersistentRegion access during GC, fix no-assert compilati… #Patch Set 4 : revert experiment #Patch Set 5 : rebased #Patch Set 6 : compile fix #
Total comments: 1
Messages
Total messages: 42 (13 generated)
haraken@chromium.org changed reviewers: + haraken@chromium.org
> Because posting will now involve creating a persistent (weak) reference, which entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). This can in some cases create lock ordering issues, leading to deadlocks. Well spotted... but would you help me understand why the MainThreadTaskRunner is the only place that causes the deadlock? I guess other WTF::bind would create CrossThreadPersistents can cause similar issues. https://codereview.chromium.org/1938313003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp (right): https://codereview.chromium.org/1938313003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp:75: if (ThreadHeap::willObjectBeLazilySwept(m_context.get())) Why was this check not needed before this CL?
On 2016/05/04 03:21:33, haraken wrote: > > Because posting will now involve creating a persistent (weak) reference, which > entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). This > can in some cases create lock ordering issues, leading to deadlocks. > > Well spotted... but would you help me understand why the MainThreadTaskRunner is > the only place that causes the deadlock? I guess other WTF::bind would create > CrossThreadPersistents can cause similar issues. > It's the only one I know of, but there could be others - we could see what happens if we assert for CrossThreadPersistent creation during GC. But lock-taking during tracing is unusual & needed to "achieve" lock inversion.
https://codereview.chromium.org/1938313003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp (right): https://codereview.chromium.org/1938313003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp:75: if (ThreadHeap::willObjectBeLazilySwept(m_context.get())) On 2016/05/04 03:21:33, haraken wrote: > > Why was this check not needed before this CL? The closure is constructed using CrossThreadWeakPersistentThisPointer<MainThreadTaskRunner>(this) and MainThreadTaskRunner & Document die at the same time, so no need to separately check the liveness of m_context here.
On 2016/05/04 05:17:25, sof wrote: > On 2016/05/04 03:21:33, haraken wrote: > > > Because posting will now involve creating a persistent (weak) reference, > which > > entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). > This > > can in some cases create lock ordering issues, leading to deadlocks. > > > > Well spotted... but would you help me understand why the MainThreadTaskRunner > is > > the only place that causes the deadlock? I guess other WTF::bind would create > > CrossThreadPersistents can cause similar issues. > > > > It's the only one I know of, but there could be others - we could see what > happens if we assert for CrossThreadPersistent creation during GC. But > lock-taking during tracing is unusual & needed to "achieve" lock inversion. The ScriptStreamer background thread posting to the Document's loading task runner is the only one showing up as accessing CrossThreadPersistentRegion while a GC runs. Which isn't a problem.
On 2016/05/04 11:39:23, sof wrote: > On 2016/05/04 05:17:25, sof wrote: > > On 2016/05/04 03:21:33, haraken wrote: > > > > Because posting will now involve creating a persistent (weak) reference, > > which > > > entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). > > This > > > can in some cases create lock ordering issues, leading to deadlocks. > > > > > > Well spotted... but would you help me understand why the > MainThreadTaskRunner > > is > > > the only place that causes the deadlock? I guess other WTF::bind would > create > > > CrossThreadPersistents can cause similar issues. > > > > > > > It's the only one I know of, but there could be others - we could see what > > happens if we assert for CrossThreadPersistent creation during GC. But > > lock-taking during tracing is unusual & needed to "achieve" lock inversion. > > The ScriptStreamer background thread posting to the Document's loading task > runner is the only one showing up as accessing CrossThreadPersistentRegion while > a GC runs. Which isn't a problem. I'm getting a bit confused -- what's a real issue of accessing CrossThreadPersistents during GCs? Once we ship the per-thread heaps, threads will be accessing CrossThreadPersistents while some thread is doing a GC. I agree that the lock in the tracing method is clearly problematic (we should remove the lock), but how is it related to the CrossThreadPersistents?
On 2016/05/04 14:25:36, haraken wrote: > On 2016/05/04 11:39:23, sof wrote: > > On 2016/05/04 05:17:25, sof wrote: > > > On 2016/05/04 03:21:33, haraken wrote: > > > > > Because posting will now involve creating a persistent (weak) reference, > > > which > > > > entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). > > > This > > > > can in some cases create lock ordering issues, leading to deadlocks. > > > > > > > > Well spotted... but would you help me understand why the > > MainThreadTaskRunner > > > is > > > > the only place that causes the deadlock? I guess other WTF::bind would > > create > > > > CrossThreadPersistents can cause similar issues. > > > > > > > > > > It's the only one I know of, but there could be others - we could see what > > > happens if we assert for CrossThreadPersistent creation during GC. But > > > lock-taking during tracing is unusual & needed to "achieve" lock inversion. > > > > The ScriptStreamer background thread posting to the Document's loading task > > runner is the only one showing up as accessing CrossThreadPersistentRegion > while > > a GC runs. Which isn't a problem. > > I'm getting a bit confused -- what's a real issue of accessing > CrossThreadPersistents during GCs? Once we ship the per-thread heaps, threads > will be accessing CrossThreadPersistents while some thread is doing a GC. > > I agree that the lock in the tracing method is clearly problematic (we should > remove the lock), but how is it related to the CrossThreadPersistents? https://bugs.chromium.org/p/chromium/issues/detail?id=590108#c8 and #c7 explains why. GCing thread can hold m_mutex on CrossThreadPersistentRegion, audio thread |m_contextGraphMutex|. If the audio thread then tries to post to the main thread, it will not be able to make progress. Nor will the marking thread if it tries to grab |m_contextGraphMutex| while tracing.
On 2016/05/04 14:31:33, sof wrote: > On 2016/05/04 14:25:36, haraken wrote: > > On 2016/05/04 11:39:23, sof wrote: > > > On 2016/05/04 05:17:25, sof wrote: > > > > On 2016/05/04 03:21:33, haraken wrote: > > > > > > Because posting will now involve creating a persistent (weak) > reference, > > > > which > > > > > entails accessing Oilpan GC data structures > (CrossThreadPersistentRegion). > > > > This > > > > > can in some cases create lock ordering issues, leading to deadlocks. > > > > > > > > > > Well spotted... but would you help me understand why the > > > MainThreadTaskRunner > > > > is > > > > > the only place that causes the deadlock? I guess other WTF::bind would > > > create > > > > > CrossThreadPersistents can cause similar issues. > > > > > > > > > > > > > It's the only one I know of, but there could be others - we could see what > > > > happens if we assert for CrossThreadPersistent creation during GC. But > > > > lock-taking during tracing is unusual & needed to "achieve" lock > inversion. > > > > > > The ScriptStreamer background thread posting to the Document's loading task > > > runner is the only one showing up as accessing CrossThreadPersistentRegion > > while > > > a GC runs. Which isn't a problem. > > > > I'm getting a bit confused -- what's a real issue of accessing > > CrossThreadPersistents during GCs? Once we ship the per-thread heaps, threads > > will be accessing CrossThreadPersistents while some thread is doing a GC. > > > > I agree that the lock in the tracing method is clearly problematic (we should > > remove the lock), but how is it related to the CrossThreadPersistents? > > https://bugs.chromium.org/p/chromium/issues/detail?id=590108#c8 and #c7 explains > why. GCing thread can hold m_mutex on CrossThreadPersistentRegion, audio thread > |m_contextGraphMutex|. If the audio thread then tries to post to the main > thread, it will not be able to make progress. Nor will the marking thread if it > tries to grab |m_contextGraphMutex| while tracing. One more clarification: Once this CL lands, can we remove the lock from the tracing method? I think the answer would be no (because both the main thread and the audio thread are still accessing the m_activeSourceNode concurrently). If so, I'm not sure if this is a right solution to the problem (= the tracing method needs to grab the lock).
On 2016/05/04 14:53:51, haraken wrote: > On 2016/05/04 14:31:33, sof wrote: > > On 2016/05/04 14:25:36, haraken wrote: > > > On 2016/05/04 11:39:23, sof wrote: > > > > On 2016/05/04 05:17:25, sof wrote: > > > > > On 2016/05/04 03:21:33, haraken wrote: > > > > > > > Because posting will now involve creating a persistent (weak) > > reference, > > > > > which > > > > > > entails accessing Oilpan GC data structures > > (CrossThreadPersistentRegion). > > > > > This > > > > > > can in some cases create lock ordering issues, leading to deadlocks. > > > > > > > > > > > > Well spotted... but would you help me understand why the > > > > MainThreadTaskRunner > > > > > is > > > > > > the only place that causes the deadlock? I guess other WTF::bind would > > > > create > > > > > > CrossThreadPersistents can cause similar issues. > > > > > > > > > > > > > > > > It's the only one I know of, but there could be others - we could see > what > > > > > happens if we assert for CrossThreadPersistent creation during GC. But > > > > > lock-taking during tracing is unusual & needed to "achieve" lock > > inversion. > > > > > > > > The ScriptStreamer background thread posting to the Document's loading > task > > > > runner is the only one showing up as accessing CrossThreadPersistentRegion > > > while > > > > a GC runs. Which isn't a problem. > > > > > > I'm getting a bit confused -- what's a real issue of accessing > > > CrossThreadPersistents during GCs? Once we ship the per-thread heaps, > threads > > > will be accessing CrossThreadPersistents while some thread is doing a GC. > > > > > > I agree that the lock in the tracing method is clearly problematic (we > should > > > remove the lock), but how is it related to the CrossThreadPersistents? > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=590108#c8 and #c7 > explains > > why. GCing thread can hold m_mutex on CrossThreadPersistentRegion, audio > thread > > |m_contextGraphMutex|. If the audio thread then tries to post to the main > > thread, it will not be able to make progress. Nor will the marking thread if > it > > tries to grab |m_contextGraphMutex| while tracing. > > One more clarification: Once this CL lands, can we remove the lock from the > tracing method? > > I think the answer would be no (because both the main thread and the audio > thread are still accessing the m_activeSourceNode concurrently). If so, I'm not > sure if this is a right solution to the problem (= the tracing method needs to > grab the lock). That's correct, the audio thread and marking thread would still conflict wrt m_activeSourceNodes access. This one would address issue 581660 only and I think we should wait with landing.
I'm trying to address the root cause here via https://codereview.chromium.org/1958583002/ - not sure if it works out in practice.
On 2016/05/06 08:46:37, sof wrote: > I'm trying to address the root cause here via > https://codereview.chromium.org/1958583002/ - not sure if it works out in > practice. Landed in r392110 and it seems to have settled in, hence this somewhat obscure MainThreadTaskRunner change isn't needed. Closing.
Message was sent while issue was closed.
Description was changed from ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for two reasons: - correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - the MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it causes problems when threads not attached to Oilpan try to post tasks to the main thread. Why? Because posting will now involve creating a persistent (weak) reference, which entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). This can in some cases create lock ordering issues, leading to deadlocks. Consequently, we've decided to avoid those by again having MainThreadTaskRunner be an off-heap object, owned by its Document. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be handled manually. R= BUG=590108 ========== to ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for two reasons: - correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - the MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it causes problems when threads not attached to Oilpan try to post tasks to the main thread. Why? Because posting will now involve creating a persistent (weak) reference, which entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). This can in some cases create lock ordering issues, leading to deadlocks. Consequently, we've decided to avoid those by again having MainThreadTaskRunner be an off-heap object, owned by its Document. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be handled manually. R= BUG=590108 ==========
Description was changed from ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for two reasons: - correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - the MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it causes problems when threads not attached to Oilpan try to post tasks to the main thread. Why? Because posting will now involve creating a persistent (weak) reference, which entails accessing Oilpan GC data structures (CrossThreadPersistentRegion). This can in some cases create lock ordering issues, leading to deadlocks. Consequently, we've decided to avoid those by again having MainThreadTaskRunner be an off-heap object, owned by its Document. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be handled manually. R= BUG=590108 ========== to ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. R= BUG=610477 ==========
sigbjornf@opera.com changed reviewers: - haraken@chromium.org
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look.
haraken@chromium.org changed reviewers: + haraken@chromium.org
This looks good. > Threads not attached to Oilpan needing to post tasks to > the main thread will have to create these persistents also. > Having that happen when a GC is in progress is hard to support, > as it risks introducing and removing persistent heap references > in ways that interfere with the GC processing the heap. Would you help me understand where the CrossThreadWeakPersistent is created? https://codereview.chromium.org/1938313003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp (right): https://codereview.chromium.org/1938313003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp:75: if (ThreadHeap::willObjectBeLazilySwept(m_context.get())) Generally I don't like willObjectBeLazilySwept, but this looks okay since it's guaranteed to be called at the end of an event loop -- the same situation as timers.
On 2016/05/26 22:04:11, haraken wrote: > This looks good. > > > Threads not attached to Oilpan needing to post tasks to > > the main thread will have to create these persistents also. > > Having that happen when a GC is in progress is hard to support, > > as it risks introducing and removing persistent heap references > > in ways that interfere with the GC processing the heap. > > Would you help me understand where the CrossThreadWeakPersistent is created? > See MainThreadTaskRunner::postTaskInternal().
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1938313003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1938313003/100001
The CQ bit was unchecked by sigbjornf@opera.com
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. R= BUG=610477 ========== to ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. C'est la vie. R= BUG=610477 ==========
Let me ask a couple of questions: - Do we need this CL even after we land https://codereview.chromium.org/2013173002/? - Is this the only place where CrossThread(Weak)Persistent is created in non-attached threads?
On 2016/05/27 07:58:35, haraken wrote: > Let me ask a couple of questions: > > - Do we need this CL even after we land > https://codereview.chromium.org/2013173002/ > Yes, we're creating far too many of these handles due to posting. And I wouldn't want to lock out the audio thread from posting while a GC runs. > - Is this the only place where CrossThread(Weak)Persistent is created in > non-attached threads? I know of no other atm.
On 2016/05/27 08:05:20, sof wrote: > On 2016/05/27 07:58:35, haraken wrote: > > Let me ask a couple of questions: > > > > - Do we need this CL even after we land > > https://codereview.chromium.org/2013173002/ > > > > Yes, we're creating far too many of these handles due to posting. And I wouldn't > want to lock out the audio thread from posting while a GC runs. > > > - Is this the only place where CrossThread(Weak)Persistent is created in > > non-attached threads? > > I know of no other atm. Does it mean that once we land this CL, we don't need https://codereview.chromium.org/2013173002/ (instead we can add some asserts to check that CrossThread(Weak)Persistent is not created by non-attached threads)? (That makes sense to me.)
On 2016/05/27 08:07:58, haraken wrote: > On 2016/05/27 08:05:20, sof wrote: > > On 2016/05/27 07:58:35, haraken wrote: > > > Let me ask a couple of questions: > > > > > > - Do we need this CL even after we land > > > https://codereview.chromium.org/2013173002/ > > > > > > > Yes, we're creating far too many of these handles due to posting. And I > wouldn't > > want to lock out the audio thread from posting while a GC runs. > > > > > - Is this the only place where CrossThread(Weak)Persistent is created in > > > non-attached threads? > > > > I know of no other atm. > > Does it mean that once we land this CL, we don't need > https://codereview.chromium.org/2013173002/ (instead we can add some asserts to > check that CrossThread(Weak)Persistent is not created by non-attached threads)? > (That makes sense to me.) We can't leave it open to non-attached thread mutation in any case.
On 2016/05/27 08:10:42, sof wrote: > On 2016/05/27 08:07:58, haraken wrote: > > On 2016/05/27 08:05:20, sof wrote: > > > On 2016/05/27 07:58:35, haraken wrote: > > > > Let me ask a couple of questions: > > > > > > > > - Do we need this CL even after we land > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. And I > > wouldn't > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is created in > > > > non-attached threads? > > > > > > I know of no other atm. > > > > Does it mean that once we land this CL, we don't need > > https://codereview.chromium.org/2013173002/ (instead we can add some asserts > to > > check that CrossThread(Weak)Persistent is not created by non-attached > threads)? > > (That makes sense to me.) > > We can't leave it open to non-attached thread mutation in any case. LGTM I'm confused because you're trying to land both this CL and https://codereview.chromium.org/2013173002/. If disallowing CrossThread(Weak)Persistent on non-attached threads is your plan (which makes sense), we don't need to land the latter, right?
On 2016/05/27 08:23:15, haraken wrote: > On 2016/05/27 08:10:42, sof wrote: > > On 2016/05/27 08:07:58, haraken wrote: > > > On 2016/05/27 08:05:20, sof wrote: > > > > On 2016/05/27 07:58:35, haraken wrote: > > > > > Let me ask a couple of questions: > > > > > > > > > > - Do we need this CL even after we land > > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. And I > > > wouldn't > > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is created in > > > > > non-attached threads? > > > > > > > > I know of no other atm. > > > > > > Does it mean that once we land this CL, we don't need > > > https://codereview.chromium.org/2013173002/ (instead we can add some asserts > > to > > > check that CrossThread(Weak)Persistent is not created by non-attached > > threads)? > > > (That makes sense to me.) > > > > We can't leave it open to non-attached thread mutation in any case. > > LGTM > > I'm confused because you're trying to land both this CL and > https://codereview.chromium.org/2013173002/. If disallowing > CrossThread(Weak)Persistent on non-attached threads is your plan (which makes > sense), we don't need to land the latter, right? I don't understand. We take out a major contributor to CrossThreadWeakPersistent<>s here, but leave it safe to use it from elsewhere by the other CL.
On 2016/05/27 08:25:48, sof wrote: > On 2016/05/27 08:23:15, haraken wrote: > > On 2016/05/27 08:10:42, sof wrote: > > > On 2016/05/27 08:07:58, haraken wrote: > > > > On 2016/05/27 08:05:20, sof wrote: > > > > > On 2016/05/27 07:58:35, haraken wrote: > > > > > > Let me ask a couple of questions: > > > > > > > > > > > > - Do we need this CL even after we land > > > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. And I > > > > wouldn't > > > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is created > in > > > > > > non-attached threads? > > > > > > > > > > I know of no other atm. > > > > > > > > Does it mean that once we land this CL, we don't need > > > > https://codereview.chromium.org/2013173002/ (instead we can add some > asserts > > > to > > > > check that CrossThread(Weak)Persistent is not created by non-attached > > > threads)? > > > > (That makes sense to me.) > > > > > > We can't leave it open to non-attached thread mutation in any case. > > > > LGTM > > > > I'm confused because you're trying to land both this CL and > > https://codereview.chromium.org/2013173002/. If disallowing > > CrossThread(Weak)Persistent on non-attached threads is your plan (which makes > > sense), we don't need to land the latter, right? > > I don't understand. We take out a major contributor to > CrossThreadWeakPersistent<>s here, but leave it safe to use it from elsewhere by > the other CL. If there is no other users (which you wrote in #27) and your plan is to disallow it (which you wrote in #29), shouldn't we add assertions instead?
On 2016/05/27 08:28:37, haraken wrote: > On 2016/05/27 08:25:48, sof wrote: > > On 2016/05/27 08:23:15, haraken wrote: > > > On 2016/05/27 08:10:42, sof wrote: > > > > On 2016/05/27 08:07:58, haraken wrote: > > > > > On 2016/05/27 08:05:20, sof wrote: > > > > > > On 2016/05/27 07:58:35, haraken wrote: > > > > > > > Let me ask a couple of questions: > > > > > > > > > > > > > > - Do we need this CL even after we land > > > > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. And > I > > > > > wouldn't > > > > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is > created > > in > > > > > > > non-attached threads? > > > > > > > > > > > > I know of no other atm. > > > > > > > > > > Does it mean that once we land this CL, we don't need > > > > > https://codereview.chromium.org/2013173002/ (instead we can add some > > asserts > > > > to > > > > > check that CrossThread(Weak)Persistent is not created by non-attached > > > > threads)? > > > > > (That makes sense to me.) > > > > > > > > We can't leave it open to non-attached thread mutation in any case. > > > > > > LGTM > > > > > > I'm confused because you're trying to land both this CL and > > > https://codereview.chromium.org/2013173002/. If disallowing > > > CrossThread(Weak)Persistent on non-attached threads is your plan (which > makes > > > sense), we don't need to land the latter, right? > > > > I don't understand. We take out a major contributor to > > CrossThreadWeakPersistent<>s here, but leave it safe to use it from elsewhere > by > > the other CL. > > If there is no other users (which you wrote in #27) and your plan is to disallow > it (which you wrote in #29), shouldn't we add assertions instead? I _know_ of no other user currently, that's all.
On 2016/05/27 08:30:27, sof wrote: > On 2016/05/27 08:28:37, haraken wrote: > > On 2016/05/27 08:25:48, sof wrote: > > > On 2016/05/27 08:23:15, haraken wrote: > > > > On 2016/05/27 08:10:42, sof wrote: > > > > > On 2016/05/27 08:07:58, haraken wrote: > > > > > > On 2016/05/27 08:05:20, sof wrote: > > > > > > > On 2016/05/27 07:58:35, haraken wrote: > > > > > > > > Let me ask a couple of questions: > > > > > > > > > > > > > > > > - Do we need this CL even after we land > > > > > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > > > > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. > And > > I > > > > > > wouldn't > > > > > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > > > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is > > created > > > in > > > > > > > > non-attached threads? > > > > > > > > > > > > > > I know of no other atm. > > > > > > > > > > > > Does it mean that once we land this CL, we don't need > > > > > > https://codereview.chromium.org/2013173002/ (instead we can add some > > > asserts > > > > > to > > > > > > check that CrossThread(Weak)Persistent is not created by non-attached > > > > > threads)? > > > > > > (That makes sense to me.) > > > > > > > > > > We can't leave it open to non-attached thread mutation in any case. > > > > > > > > LGTM > > > > > > > > I'm confused because you're trying to land both this CL and > > > > https://codereview.chromium.org/2013173002/. If disallowing > > > > CrossThread(Weak)Persistent on non-attached threads is your plan (which > > makes > > > > sense), we don't need to land the latter, right? > > > > > > I don't understand. We take out a major contributor to > > > CrossThreadWeakPersistent<>s here, but leave it safe to use it from > elsewhere > > by > > > the other CL. > > > > If there is no other users (which you wrote in #27) and your plan is to > disallow > > it (which you wrote in #29), shouldn't we add assertions instead? > > I _know_ of no other user currently, that's all. Either way, disallowing it would be a right way to go, so this CL looks good to me.
On 2016/05/27 08:31:53, haraken wrote: > On 2016/05/27 08:30:27, sof wrote: > > On 2016/05/27 08:28:37, haraken wrote: > > > On 2016/05/27 08:25:48, sof wrote: > > > > On 2016/05/27 08:23:15, haraken wrote: > > > > > On 2016/05/27 08:10:42, sof wrote: > > > > > > On 2016/05/27 08:07:58, haraken wrote: > > > > > > > On 2016/05/27 08:05:20, sof wrote: > > > > > > > > On 2016/05/27 07:58:35, haraken wrote: > > > > > > > > > Let me ask a couple of questions: > > > > > > > > > > > > > > > > > > - Do we need this CL even after we land > > > > > > > > > https://codereview.chromium.org/2013173002/ > > > > > > > > > > > > > > > > > > > > > > > > > Yes, we're creating far too many of these handles due to posting. > > And > > > I > > > > > > > wouldn't > > > > > > > > want to lock out the audio thread from posting while a GC runs. > > > > > > > > > > > > > > > > > - Is this the only place where CrossThread(Weak)Persistent is > > > created > > > > in > > > > > > > > > non-attached threads? > > > > > > > > > > > > > > > > I know of no other atm. > > > > > > > > > > > > > > Does it mean that once we land this CL, we don't need > > > > > > > https://codereview.chromium.org/2013173002/ (instead we can add some > > > > asserts > > > > > > to > > > > > > > check that CrossThread(Weak)Persistent is not created by > non-attached > > > > > > threads)? > > > > > > > (That makes sense to me.) > > > > > > > > > > > > We can't leave it open to non-attached thread mutation in any case. > > > > > > > > > > LGTM > > > > > > > > > > I'm confused because you're trying to land both this CL and > > > > > https://codereview.chromium.org/2013173002/. If disallowing > > > > > CrossThread(Weak)Persistent on non-attached threads is your plan (which > > > makes > > > > > sense), we don't need to land the latter, right? > > > > > > > > I don't understand. We take out a major contributor to > > > > CrossThreadWeakPersistent<>s here, but leave it safe to use it from > > elsewhere > > > by > > > > the other CL. > > > > > > If there is no other users (which you wrote in #27) and your plan is to > > disallow > > > it (which you wrote in #29), shouldn't we add assertions instead? > > > > I _know_ of no other user currently, that's all. > > Either way, disallowing it would be a right way to go, so this CL looks good to > me. We don't know if that's a viable constraint to impose just yet, but a separate issue from this CL.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1938313003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1938313003/100001
Message was sent while issue was closed.
Description was changed from ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. C'est la vie. R= BUG=610477 ========== to ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. C'est la vie. R= BUG=610477 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. C'est la vie. R= BUG=610477 ========== to ========== Move MainThreadTaskRunner off Oilpan heap to simplify posting. Having the Document's MainThreadTaskRunner on the Oilpan heap is preferable for three reasons: - Correctly accounts for the MainThreadTaskRunner::m_context back reference, by having it be traced Member<>. - The MainThreadTaskRunner must not perform tasks when it (and the Document) is in the process of being swept. By having the posted tasks keep a weak persistent reference to MainThreadTaskRunner, the Oilpan GC will ensure that the weak references will be cleared once MainThreadTaskRunner has been deemed garbage. - Similarly for the timer-initiated running of a MainThreadTaskRunner's pending tasks. The Timer<> abstraction takes care of not firing a timer if its owner is an Oilpan heap object that's about to be swept. But it is not without downsides: - A CrossThreadWeakPersistent<> has to be created for every task closure posted to the main thread, and copying that persistent reference around while creating the closure, something that is not without overhead. - Threads not attached to Oilpan needing to post tasks to the main thread will have to create these persistents also. Having that happen when a GC is in progress is hard to support, as it risks introducing and removing persistent heap references in ways that interfere with the GC processing the heap. The latter point is sufficient reason not to require the allocation of CrossThreadWeakPersistent<>s when posting main thread tasks, hence MainThreadTaskRunner is moved off the Oilpan heap. By doing so, the benefits above that the Oilpan GC infrastructure provided "for free" have to be taken care of manually. C'est la vie. R= BUG=610477 Committed: https://crrev.com/93ddd5fff7fbe018efc1df2be3fe907103314422 Cr-Commit-Position: refs/heads/master@{#396432} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/93ddd5fff7fbe018efc1df2be3fe907103314422 Cr-Commit-Position: refs/heads/master@{#396432}
Message was sent while issue was closed.
On 2016/05/27 08:33:12, sof wrote: > On 2016/05/27 08:31:53, haraken wrote: > > On 2016/05/27 08:30:27, sof wrote: ... > > > > > > > > If there is no other users (which you wrote in #27) and your plan is to > > > disallow > > > > it (which you wrote in #29), shouldn't we add assertions instead? > > > > > > I _know_ of no other user currently, that's all. > > > > Either way, disallowing it would be a right way to go, so this CL looks good > to > > me. > > We don't know if that's a viable constraint to impose just yet, but a separate > issue from this CL. https://codereview.chromium.org/2019673002/ shows up audio thread usage of a CTP (AudioParamTimeline::ParamEvent) It looks like the background thread usage by CanvasAsyncBlobCreator could be another case; I think it'd be too strict to not allow CTP usage - keeping a lock while marking & weak processing would be a reasonable initial implementation, imo. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
