mlippautz@: Can you please take a look and let me know if the general approach ...
3 years, 7 months ago
(2017-05-24 14:34:56 UTC)
#4
mlippautz@: Can you please take a look and
let me know if the general approach to using wrapper tracing
here makes sense.
Once we confirm that the approach is sound, the I will convert other worklets to
wrapper tracing where applicable.
+haraken for a general review. Wrapper tracing looks good % comments. https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): ...
3 years, 7 months ago
(2017-05-24 14:42:44 UTC)
#6
3 years, 7 months ago
(2017-05-24 16:53:22 UTC)
#10
Dry run: This issue passed the CQ dry run.
haraken
LGTM https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h (right): https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h#newcode49 third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h:49: AnimatorList animators_; Do you need to trace both ...
3 years, 7 months ago
(2017-05-25 04:22:12 UTC)
#11
LGTM
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h
(right):
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h:49:
AnimatorList animators_;
Do you need to trace both animator_definitions_ and animators_? It's harmless to
trace both but it's sufficient to trace AnimatorDefinition once. Maybe would it
be sufficient to trace only animators_? Then the Animator object will trace into
the corresponding AnimatorDefinition.
majidvp
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
3 years, 6 months ago
(2017-05-25 20:11:10 UTC)
#12
Description was changed from ========== Use wrapper tracing for worklets. Many worklet provide a way ...
3 years, 6 months ago
(2017-05-25 20:16:48 UTC)
#14
Description was changed from
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
WIP: currently done for animation worklets but extends to others
BUG=
==========
to
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
BUG=726457
==========
majidvp
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp#newcode54 third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:54: visitor->TraceWrappers(definition.value); On 2017/05/24 14:42:43, Michael Lippautz wrote: > Missing ...
3 years, 6 months ago
(2017-05-25 20:16:57 UTC)
#15
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp
(right):
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:54:
visitor->TraceWrappers(definition.value);
On 2017/05/24 14:42:43, Michael Lippautz wrote:
> Missing dispatch to parent:
> ThreadedWorkletGlobalScope::TraceWrappers(visitor);
Done.
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (left):
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/Animator.cpp:19:
DEFINE_TRACE(Animator) {
On 2017/05/24 14:42:43, Michael Lippautz wrote:
> Why don't we need to trace the definition for Oilpan?
Fixed. We actually do.
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h
(right):
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h:49:
AnimatorList animators_;
On 2017/05/25 04:22:12, haraken wrote:
>
> Do you need to trace both animator_definitions_ and animators_? It's harmless
to
> trace both but it's sufficient to trace AnimatorDefinition once. Maybe would
it
> be sufficient to trace only animators_? Then the Animator object will trace
into
> the corresponding AnimatorDefinition.
hmmmm, I think it is better to trace them both here.
If you want to avoid overhead of tracing things twice (*) then I propose to not
trace the definition from animator. My reasoning is that both animators and
definitions are owned by the scope. The animator reference to its definition is
not owning one. So it seems more natural to trace along the ownership graph.
(*) I am not sure why we care about the tracing cost. In particular, for oilpan
tracing it is very typical to have the
same object being traced via Member<> from multiple path and that
is not very concerning. Is there anything special about wrapper
tracing to warrant being careful as opposed to a simpler guideline
of trace every wrapper.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 6 months ago
(2017-05-25 21:35:24 UTC)
#16
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/379319)
3 years, 6 months ago
(2017-05-25 21:35:25 UTC)
#17
3 years, 6 months ago
(2017-05-26 02:14:54 UTC)
#18
On 2017/05/25 20:16:57, majidvp wrote:
>
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
> File
>
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp
> (right):
>
>
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
>
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:54:
> visitor->TraceWrappers(definition.value);
> On 2017/05/24 14:42:43, Michael Lippautz wrote:
> > Missing dispatch to parent:
> > ThreadedWorkletGlobalScope::TraceWrappers(visitor);
>
> Done.
>
>
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (left):
>
>
https://codereview.chromium.org/2903703003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/compositorworker/Animator.cpp:19:
> DEFINE_TRACE(Animator) {
> On 2017/05/24 14:42:43, Michael Lippautz wrote:
> > Why don't we need to trace the definition for Oilpan?
>
> Fixed. We actually do.
>
>
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h
> (right):
>
>
https://codereview.chromium.org/2903703003/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.h:49:
> AnimatorList animators_;
> On 2017/05/25 04:22:12, haraken wrote:
> >
> > Do you need to trace both animator_definitions_ and animators_? It's
harmless
> to
> > trace both but it's sufficient to trace AnimatorDefinition once. Maybe would
> it
> > be sufficient to trace only animators_? Then the Animator object will trace
> into
> > the corresponding AnimatorDefinition.
>
> hmmmm, I think it is better to trace them both here.
>
> If you want to avoid overhead of tracing things twice (*) then I propose to
not
> trace the definition from animator. My reasoning is that both animators and
> definitions are owned by the scope. The animator reference to its definition
is
> not owning one. So it seems more natural to trace along the ownership graph.
>
> (*) I am not sure why we care about the tracing cost. In particular, for
oilpan
> tracing it is very typical to have the
> same object being traced via Member<> from multiple path and that
> is not very concerning. Is there anything special about wrapper
> tracing to warrant being careful as opposed to a simpler guideline
> of trace every wrapper.
First of all, this is not a big deal. Feel free to land the CL.
The goal of the wrapper tracing is not to create a perfect graph (then we should
just use the oilpan's graph) but to create a graph that represents reachability
between wrappers (instead of tracing all edges just in case). So if the object
has an explicit ownership, it's sufficient to trace only the ownership edge.
majidvp
haraken@, mlippautz@: I wonder if you know the answer to this. The current patch adds ...
3 years, 6 months ago
(2017-05-31 19:20:37 UTC)
#19
haraken@, mlippautz@: I wonder if you know the answer to this.
The current patch adds wrapper tracing for object owned by the
WorkletGlobalScope. But at the moment it is failing the following
GC unit test for paint worklet:
PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
My guess is that although we are tracing paint definitions from paint
global scope but we never really trace the worklet global scope itself.
The worklet global scope is kept alive via a Persistent reference.
So from V8 GC point of view, no one is actually using these wrapped objects
so it goes ahead and collect them despite being owned by the global scope.
Any idea how to fix this?
[1]
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
Michael Lippautz
On 2017/05/31 19:20:37, majidvp wrote: > haraken@, mlippautz@: I wonder if you know the answer ...
3 years, 6 months ago
(2017-05-31 20:06:39 UTC)
#20
On 2017/05/31 19:20:37, majidvp wrote:
> haraken@, mlippautz@: I wonder if you know the answer to this.
>
> The current patch adds wrapper tracing for object owned by the
> WorkletGlobalScope. But at the moment it is failing the following
> GC unit test for paint worklet:
> PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
>
> My guess is that although we are tracing paint definitions from paint
> global scope but we never really trace the worklet global scope itself.
> The worklet global scope is kept alive via a Persistent reference.
>
Can you point me to the reference you are talking about? In general:
v8::Persistent don't provide entry points into the graph, only
blink::ScriptWrappable_s do. Now, when I initially looked at this change I saw
that WorkletGlobalScope is a ScriptWrappable, so all the *WorkletGlobalScope
should be too. The JS wrapper object should then be the entry point for tracing.
Another idea would be that V8's Scavenger gets rid of them because they are
supposedly treated as independent. Can you try adding "DependentLifetime" to
WorkletGlobalScope.idl (or their more specific IDL file, e.g.
AudioWorkletGlobalScope.idl)?
>
> So from V8 GC point of view, no one is actually using these wrapped objects
> so it goes ahead and collect them despite being owned by the global scope.
> Any idea how to fix this?
>
> [1]
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
haraken
On 2017/05/31 20:06:39, Michael Lippautz wrote: > On 2017/05/31 19:20:37, majidvp wrote: > > haraken@, ...
3 years, 6 months ago
(2017-06-01 02:05:27 UTC)
#21
On 2017/05/31 20:06:39, Michael Lippautz wrote:
> On 2017/05/31 19:20:37, majidvp wrote:
> > haraken@, mlippautz@: I wonder if you know the answer to this.
> >
> > The current patch adds wrapper tracing for object owned by the
> > WorkletGlobalScope. But at the moment it is failing the following
> > GC unit test for paint worklet:
> > PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
> >
> > My guess is that although we are tracing paint definitions from paint
> > global scope but we never really trace the worklet global scope itself.
> > The worklet global scope is kept alive via a Persistent reference.
> >
>
> Can you point me to the reference you are talking about? In general:
> v8::Persistent don't provide entry points into the graph, only
> blink::ScriptWrappable_s do. Now, when I initially looked at this change I saw
> that WorkletGlobalScope is a ScriptWrappable, so all the *WorkletGlobalScope
> should be too. The JS wrapper object should then be the entry point for
tracing.
>
>
> Another idea would be that V8's Scavenger gets rid of them because they are
> supposedly treated as independent. Can you try adding "DependentLifetime" to
> WorkletGlobalScope.idl (or their more specific IDL file, e.g.
> AudioWorkletGlobalScope.idl)?
I guess this could be the root cause of the failure.
We must add [DependentLifetime] on IDL files of DOM objects that have
TraceWrappers or are traced by TraceWrappers. This is really error-prone and we
should fix it at some point.
Michael: In short term, would it be possible to add a DCHECK to verify that DOM
wrappers that have TraceWrappers or are traced by TraceWrappers are annotated
with [DependentLifetime]?
>
> >
> > So from V8 GC point of view, no one is actually using these wrapped objects
> > so it goes ahead and collect them despite being owned by the global scope.
> > Any idea how to fix this?
> >
> > [1]
> >
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
majidvp
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
3 years, 6 months ago
(2017-06-01 17:46:17 UTC)
#22
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/223574) ios-device-xcode-clang on ...
3 years, 6 months ago
(2017-06-01 17:50:04 UTC)
#25
On 2017/06/01 02:05:27, haraken wrote: > On 2017/05/31 20:06:39, Michael Lippautz wrote: > > On ...
3 years, 6 months ago
(2017-06-01 19:42:37 UTC)
#28
On 2017/06/01 02:05:27, haraken wrote:
> On 2017/05/31 20:06:39, Michael Lippautz wrote:
> > On 2017/05/31 19:20:37, majidvp wrote:
> > > haraken@, mlippautz@: I wonder if you know the answer to this.
> > >
> > > The current patch adds wrapper tracing for object owned by the
> > > WorkletGlobalScope. But at the moment it is failing the following
> > > GC unit test for paint worklet:
> > > PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
> > >
> > > My guess is that although we are tracing paint definitions from paint
> > > global scope but we never really trace the worklet global scope itself.
> > > The worklet global scope is kept alive via a Persistent reference.
> > >
> >
> > Can you point me to the reference you are talking about? In general:
> > v8::Persistent don't provide entry points into the graph, only
> > blink::ScriptWrappable_s do. Now, when I initially looked at this change I
saw
> > that WorkletGlobalScope is a ScriptWrappable, so all the *WorkletGlobalScope
> > should be too. The JS wrapper object should then be the entry point for
> tracing.
> >
> >
> > Another idea would be that V8's Scavenger gets rid of them because they are
> > supposedly treated as independent. Can you try adding "DependentLifetime" to
> > WorkletGlobalScope.idl (or their more specific IDL file, e.g.
> > AudioWorkletGlobalScope.idl)?
>
> I guess this could be the root cause of the failure.
>
> We must add [DependentLifetime] on IDL files of DOM objects that have
> TraceWrappers or are traced by TraceWrappers. This is really error-prone and
we
> should fix it at some point.
>
> Michael: In short term, would it be possible to add a DCHECK to verify that
DOM
> wrappers that have TraceWrappers or are traced by TraceWrappers are annotated
> with [DependentLifetime]?
>
>
> >
> > >
> > > So from V8 GC point of view, no one is actually using these wrapped
objects
> > > so it goes ahead and collect them despite being owned by the global scope.
> > > Any idea how to fix this?
> > >
> > > [1]
> > >
> >
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
Thanks for suggestions. Adding [DependentLifetime] didn't fix the problem
but adding both [DependentLifetime, ActiveScriptWrappable] did the
trick. This is also what we do for worker global scope.
It turned out the global scope (worker or worklet) is actually not wrapped
[1] by any wrapper which is why I think it needs to be an ActiveScriptWrappable.
I had a chat with ikilpatrick@ and returning true on
|WorkletGlobalScope::HasPendingActivity| is reasonable given the
current semantic for worklets.
[1] WorkerGlobalScope must never be wrapped with wrap method.
The global object of ECMAScript environment is used as the
wrapper.
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/w...
majidvp
haraken@ PTAL
3 years, 6 months ago
(2017-06-01 19:42:59 UTC)
#29
haraken@ PTAL
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 6 months ago
(2017-06-02 00:14:22 UTC)
#30
3 years, 6 months ago
(2017-06-02 00:14:24 UTC)
#31
Dry run: This issue passed the CQ dry run.
haraken
Would it be possible to split the patch into two parts? - Make GlobalScopes ActiveScriptWrappable ...
3 years, 6 months ago
(2017-06-02 02:32:25 UTC)
#32
Would it be possible to split the patch into two parts?
- Make GlobalScopes ActiveScriptWrappable
- Introduce the wrapper tracing
https://codereview.chromium.org/2903703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp (right):
https://codereview.chromium.org/2903703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp:49: return true;
HasPendingActivity *must* return false in a finite time period. Otherwise this
object will leak (V8 will keep the wrapper alive forever).
I guess we should start returning false after WorkletGlobalScope gets detached.
(Note: In practice your code won't cause leaks because I added a hack to
forcibly collect wrappers that are associated with detached contexts. However,
it's not nice to write code that relies on the hack.)
majidvp
On 2017/06/02 02:32:25, haraken wrote: > Would it be possible to split the patch into ...
3 years, 6 months ago
(2017-06-02 17:52:41 UTC)
#33
On 2017/06/02 02:32:25, haraken wrote:
> Would it be possible to split the patch into two parts?
>
> - Make GlobalScopes ActiveScriptWrappable
> - Introduce the wrapper tracing
>
Done.
- Make GlobalScopes ActiveScriptWrappable:
https://chromium-review.googlesource.com/c/522169/
- Introduce the wrapper tracing: this CL
>
https://codereview.chromium.org/2903703003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp (right):
>
>
https://codereview.chromium.org/2903703003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp:49: return true;
>
> HasPendingActivity *must* return false in a finite time period. Otherwise this
> object will leak (V8 will keep the wrapper alive forever).
>
> I guess we should start returning false after WorkletGlobalScope gets
detached.
>
> (Note: In practice your code won't cause leaks because I added a hack to
> forcibly collect wrappers that are associated with detached contexts. However,
> it's not nice to write code that relies on the hack.)
Done. Made it so that we no longer keep the wrapper alive once context is
destroyed.
(It is in the other CL)
majidvp
Description was changed from ========== Use wrapper tracing for worklets. Many worklet provide a way ...
3 years, 6 months ago
(2017-06-02 17:53:32 UTC)
#34
Description was changed from
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
BUG=726457
==========
to
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
This depends on having worklet global scope ActiveScriptWrappable
which is here: https://chromium-review.googlesource.com/c/522169/
BUG=726457
==========
majidvp
The CQ bit was checked by majidvp@chromium.org
3 years, 6 months ago
(2017-06-06 12:14:30 UTC)
#35
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496751270011730, "parent_rev": "8588f634d6722cf9dc6a48ddc201d8121c2c57f2", "commit_rev": "303c5ab1608b16d3e4b793557718b3666a720fff"}
3 years, 6 months ago
(2017-06-06 13:55:28 UTC)
#38
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496751270011730,
"parent_rev": "8588f634d6722cf9dc6a48ddc201d8121c2c57f2", "commit_rev":
"303c5ab1608b16d3e4b793557718b3666a720fff"}
commit-bot: I haz the power
Description was changed from ========== Use wrapper tracing for worklets. Many worklet provide a way ...
3 years, 6 months ago
(2017-06-06 13:55:36 UTC)
#39
Message was sent while issue was closed.
Description was changed from
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
This depends on having worklet global scope ActiveScriptWrappable
which is here: https://chromium-review.googlesource.com/c/522169/
BUG=726457
==========
to
==========
Use wrapper tracing for worklets.
Many worklet provide a way for scripts to register components (e.g., paint,
animator, processor) in the worklet global scope. These components own v8
objects so they should participate in wrapper tracing to ensure accurate and
timely garbage collection.
This depends on having worklet global scope ActiveScriptWrappable
which is here: https://chromium-review.googlesource.com/c/522169/
BUG=726457
Review-Url: https://codereview.chromium.org/2903703003
Cr-Commit-Position: refs/heads/master@{#477275}
Committed:
https://chromium.googlesource.com/chromium/src/+/303c5ab1608b16d3e4b793557718...
==========
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/303c5ab1608b16d3e4b793557718b3666a720fff
3 years, 6 months ago
(2017-06-06 13:55:39 UTC)
#40
On 2017/06/01 02:05:27, haraken wrote: > On 2017/05/31 20:06:39, Michael Lippautz wrote: > > On ...
3 years, 6 months ago
(2017-06-21 12:06:17 UTC)
#41
Message was sent while issue was closed.
On 2017/06/01 02:05:27, haraken wrote:
> On 2017/05/31 20:06:39, Michael Lippautz wrote:
> > On 2017/05/31 19:20:37, majidvp wrote:
> > > haraken@, mlippautz@: I wonder if you know the answer to this.
> > >
> > > The current patch adds wrapper tracing for object owned by the
> > > WorkletGlobalScope. But at the moment it is failing the following
> > > GC unit test for paint worklet:
> > > PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
> > >
> > > My guess is that although we are tracing paint definitions from paint
> > > global scope but we never really trace the worklet global scope itself.
> > > The worklet global scope is kept alive via a Persistent reference.
> > >
> >
> > Can you point me to the reference you are talking about? In general:
> > v8::Persistent don't provide entry points into the graph, only
> > blink::ScriptWrappable_s do. Now, when I initially looked at this change I
saw
> > that WorkletGlobalScope is a ScriptWrappable, so all the *WorkletGlobalScope
> > should be too. The JS wrapper object should then be the entry point for
> tracing.
> >
> >
> > Another idea would be that V8's Scavenger gets rid of them because they are
> > supposedly treated as independent. Can you try adding "DependentLifetime" to
> > WorkletGlobalScope.idl (or their more specific IDL file, e.g.
> > AudioWorkletGlobalScope.idl)?
>
> I guess this could be the root cause of the failure.
>
> We must add [DependentLifetime] on IDL files of DOM objects that have
> TraceWrappers or are traced by TraceWrappers. This is really error-prone and
we
> should fix it at some point.
>
> Michael: In short term, would it be possible to add a DCHECK to verify that
DOM
> wrappers that have TraceWrappers or are traced by TraceWrappers are annotated
> with [DependentLifetime]?
^^^ Michael: Do you have a cycle to work on the short-term fix at the moment?
Michael Lippautz
On 2017/06/21 12:06:17, haraken wrote: > On 2017/06/01 02:05:27, haraken wrote: > > On 2017/05/31 ...
3 years, 6 months ago
(2017-06-21 19:26:47 UTC)
#42
Message was sent while issue was closed.
On 2017/06/21 12:06:17, haraken wrote:
> On 2017/06/01 02:05:27, haraken wrote:
> > On 2017/05/31 20:06:39, Michael Lippautz wrote:
> > > On 2017/05/31 19:20:37, majidvp wrote:
> > > > haraken@, mlippautz@: I wonder if you know the answer to this.
> > > >
> > > > The current patch adds wrapper tracing for object owned by the
> > > > WorkletGlobalScope. But at the moment it is failing the following
> > > > GC unit test for paint worklet:
> > > > PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
> > > >
> > > > My guess is that although we are tracing paint definitions from paint
> > > > global scope but we never really trace the worklet global scope itself.
> > > > The worklet global scope is kept alive via a Persistent reference.
> > > >
> > >
> > > Can you point me to the reference you are talking about? In general:
> > > v8::Persistent don't provide entry points into the graph, only
> > > blink::ScriptWrappable_s do. Now, when I initially looked at this change I
> saw
> > > that WorkletGlobalScope is a ScriptWrappable, so all the
*WorkletGlobalScope
> > > should be too. The JS wrapper object should then be the entry point for
> > tracing.
> > >
> > >
> > > Another idea would be that V8's Scavenger gets rid of them because they
are
> > > supposedly treated as independent. Can you try adding "DependentLifetime"
to
> > > WorkletGlobalScope.idl (or their more specific IDL file, e.g.
> > > AudioWorkletGlobalScope.idl)?
> >
> > I guess this could be the root cause of the failure.
> >
> > We must add [DependentLifetime] on IDL files of DOM objects that have
> > TraceWrappers or are traced by TraceWrappers. This is really error-prone and
> we
> > should fix it at some point.
> >
> > Michael: In short term, would it be possible to add a DCHECK to verify that
> DOM
> > wrappers that have TraceWrappers or are traced by TraceWrappers are
annotated
> > with [DependentLifetime]?
>
> ^^^ Michael: Do you have a cycle to work on the short-term fix at the moment?
I tried last week but couldn't really find a nice spot to put it. Essentially,
we need to allow regular tracing for those without DependentLifetime as far as I
can see, because V8 is just passing them over to Blink.
Thus, the check should be something along the lines
if (!DependentLifetime(T)) {
DCHECK(std::is_same<decltype(&T::Foo),decltype(&TraceWrapperBase::TraceWrappers)>::value);
}
However, I failed to find a place where we have both, the type T, and
WrapperTypeInfo available. There's a cyclic include between WrapperVisitor.h and
WrapperTypeInfo.h that would need to be resolved first, so that we can use
WrapperTypeInfo in WrapperVisitor.
I will dedicate another day this week for this problem. Maybe we can find a
better way.
haraken
On 2017/06/21 19:26:47, Michael Lippautz wrote: > On 2017/06/21 12:06:17, haraken wrote: > > On ...
3 years, 6 months ago
(2017-06-22 02:39:09 UTC)
#43
Message was sent while issue was closed.
On 2017/06/21 19:26:47, Michael Lippautz wrote:
> On 2017/06/21 12:06:17, haraken wrote:
> > On 2017/06/01 02:05:27, haraken wrote:
> > > On 2017/05/31 20:06:39, Michael Lippautz wrote:
> > > > On 2017/05/31 19:20:37, majidvp wrote:
> > > > > haraken@, mlippautz@: I wonder if you know the answer to this.
> > > > >
> > > > > The current patch adds wrapper tracing for object owned by the
> > > > > WorkletGlobalScope. But at the moment it is failing the following
> > > > > GC unit test for paint worklet:
> > > > > PaintWorkletTest.GarbageCollectionOfCSSPaintDefinition [1]
> > > > >
> > > > > My guess is that although we are tracing paint definitions from paint
> > > > > global scope but we never really trace the worklet global scope
itself.
> > > > > The worklet global scope is kept alive via a Persistent reference.
> > > > >
> > > >
> > > > Can you point me to the reference you are talking about? In general:
> > > > v8::Persistent don't provide entry points into the graph, only
> > > > blink::ScriptWrappable_s do. Now, when I initially looked at this change
I
> > saw
> > > > that WorkletGlobalScope is a ScriptWrappable, so all the
> *WorkletGlobalScope
> > > > should be too. The JS wrapper object should then be the entry point for
> > > tracing.
> > > >
> > > >
> > > > Another idea would be that V8's Scavenger gets rid of them because they
> are
> > > > supposedly treated as independent. Can you try adding
"DependentLifetime"
> to
> > > > WorkletGlobalScope.idl (or their more specific IDL file, e.g.
> > > > AudioWorkletGlobalScope.idl)?
> > >
> > > I guess this could be the root cause of the failure.
> > >
> > > We must add [DependentLifetime] on IDL files of DOM objects that have
> > > TraceWrappers or are traced by TraceWrappers. This is really error-prone
and
> > we
> > > should fix it at some point.
> > >
> > > Michael: In short term, would it be possible to add a DCHECK to verify
that
> > DOM
> > > wrappers that have TraceWrappers or are traced by TraceWrappers are
> annotated
> > > with [DependentLifetime]?
> >
> > ^^^ Michael: Do you have a cycle to work on the short-term fix at the
moment?
>
> I tried last week but couldn't really find a nice spot to put it. Essentially,
> we need to allow regular tracing for those without DependentLifetime as far as
I
> can see, because V8 is just passing them over to Blink.
>
> Thus, the check should be something along the lines
> if (!DependentLifetime(T)) {
>
>
DCHECK(std::is_same<decltype(&T::Foo),decltype(&TraceWrapperBase::TraceWrappers)>::value);
> }
>
> However, I failed to find a place where we have both, the type T, and
> WrapperTypeInfo available. There's a cyclic include between WrapperVisitor.h
and
> WrapperTypeInfo.h that would need to be resolved first, so that we can use
> WrapperTypeInfo in WrapperVisitor.
>
> I will dedicate another day this week for this problem. Maybe we can find a
> better way.
Thanks Michael!
We won't need to include the type T, right? To use:
scriptWrappable->GetWrapperTypeInfo()->isDependent()
it would be enough to include ScriptWrappable.h and WrapperTypeInfo.h in
ScriptWrappableVisitor.cpp... ?
Issue 2903703003: Use wrapper tracing for worklets.
(Closed)
Created 3 years, 7 months ago by majidvp
Modified 3 years, 6 months ago
Reviewers: Michael Lippautz, haraken
Base URL:
Comments: 7
Chromium Code Reviews
Issue
2903703003:
Use wrapper tracing for worklets. (Closed)
