Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be observed
by LifecycleObservers and notified when the observing context changes its lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and checking their
pending activities when the main thread posts a message to the worker. After this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending activities.
Given that the number of ScriptWrappables is larger than the number of ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/

Committed: https://crrev.com/485c63ccd15b1e6efe6baf67010da6ea2db94e0d
Cr-Commit-Position: refs/heads/master@{#374584}

[haraken, 2016-01-22 17:12:42]

Description was changed from

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

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

to

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override
hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending
activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be
observed
by LifecycleObservers and notified when the observing context changes its
lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to
ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a
worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and
checking their
pending activities when the main thread posts a message to the worker. After
this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending
activities.
Given that the number of ScriptWrappables is larger than the number of
ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't
know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is
limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending
timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have
any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/
==========

[haraken, 2016-01-22 17:12:42]

haraken@chromium.org changed reviewers:
+ kinuko@chromium.org, vivekg@chromium.org

[haraken, 2016-01-22 17:15:44]

kinuko-san: Would you take a quick look at this CL? If the approach looks okay,
I'll land a couple of preparation CLs before landing this one.

The only concern is that after this CL, each postMessage starts scanning a list
of ScriptWrappables (instead of a list of ActiveDOMObjects). The number of
ScriptWrappables is larger than ActiveDOMObjects. I hope the number of
ScriptWrappables in workers is limited and thus the overhead wouldn't be an
issue in common cases, but I'm not quite sure.

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:482: /* &&
toExecutionContext(v8::Object::CreationContext(wrapper)) == m_executionContext
*/

To enable this condition, I need to implement the V8 API (i.e.,
v8::Object::CreationContext). I'll do that before landing this CL.

(Note: Even if we don't enable the condition, it will not affect the correctness
-- it will just delay collecting a worker object of compositor workers, where
multiple ExecutionContexts are associated with one isolate.)

[kinuko, 2016-01-25 04:06:25]

As this doesn't seem to change the existing policy of how it triggers GC it
looks good to me. Could we add a UMA so that we can keep watching the # of
script wrappables visited?

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:468: {
Could we count how many # of handles this class visited and report it as a UMA
so that we can take a look at it while it's in Canary & whenever we have
performance concern?

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:471: if
(m_pendingActivityFound)
Would there be a way to exit the visitor loop once we found pending activity? 
...looks like we need v8 change which may not be worth doing unless we see perf
issue.

[haraken, 2016-01-25 04:08:42]

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:468: {
On 2016/01/25 04:06:25, kinuko wrote:
> Could we count how many # of handles this class visited and report it as a UMA
> so that we can take a look at it while it's in Canary & whenever we have
> performance concern?

Sounds nice. Will do.

https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:471: if
(m_pendingActivityFound)
On 2016/01/25 04:06:25, kinuko wrote:
> Would there be a way to exit the visitor loop once we found pending activity? 
> ...looks like we need v8 change which may not be worth doing unless we see
perf
> issue.

Yeah, in the current V8 API, we don't have a way to exit the loop earlier :/

[haraken, 2016-02-08 05:42:45]

kinuko-san: PTAL

On 2016/01/25 04:08:42, haraken wrote:
>
https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/1609343002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:468: {
> On 2016/01/25 04:06:25, kinuko wrote:
> > Could we count how many # of handles this class visited and report it as a
UMA
> > so that we can take a look at it while it's in Canary & whenever we have
> > performance concern?
> 
> Sounds nice. Will do.
> 

Done.

https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:497: /* &&
toExecutionContext(wrapper->creationContext()) == m_executionContext */

This needs a new V8 API (v8::Object::CreationContext), but the V8 team doesn't
want to introduce the API because we're deprecating CreationContext. Our plan is
to deprecate CreationContext from V8 by improving V8 binding so that each V8
wrapper understands the context from which the wrapper has been created.
However, it needs a substantial amount of work. So I'd like to land this CL with
TODO at the moment.

[haraken, 2016-02-09 00:05:00]

kinuko-san: friendly ping?

[kinuko, 2016-02-09 02:23:19]

LGTM, sorry for slow response

[haraken, 2016-02-09 03:26:38]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-02-09 03:27:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609343002/60001

[commit-bot: I haz the power, 2016-02-09 03:41:50]

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

[commit-bot: I haz the power, 2016-02-09 03:41:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haraken, 2016-02-09 04:06:45]

haraken@chromium.org changed reviewers:
+ isherman@chromium.org

[haraken, 2016-02-09 04:06:45]

isherman@: PTAL at histogram.xml.

[Ilya Sherman, 2016-02-09 22:07:00]

https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:519:
scanPendingActivityHistogram.count(WTF::currentTimeMS() - startTime);
Hrm, you seem to be passing a double into a function that expects an int.  Does
this not generate a compiler error -- and if not, what am I missing?

https://codereview.chromium.org/1609343002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1609343002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3178: +<histogram
name="Blink.ScanPendingActivityHistogram" units="ms">
nit: s/Histogram/Duration

[haraken, 2016-02-09 23:59:00]

On 2016/02/09 22:07:00, Ilya Sherman wrote:
>
https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/1609343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:519:
> scanPendingActivityHistogram.count(WTF::currentTimeMS() - startTime);
> Hrm, you seem to be passing a double into a function that expects an int. 
Does
> this not generate a compiler error -- and if not, what am I missing?

Interesting. It doesn't cause a compile error. We already have a place that
passes double to CustomCountHistogram.count.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Added static_cast<int> just in case.

> 
>
https://codereview.chromium.org/1609343002/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1609343002/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:3178: +<histogram
> name="Blink.ScanPendingActivityHistogram" units="ms">
> nit: s/Histogram/Duration

Done.

[Ilya Sherman, 2016-02-10 00:53:13]

Histograms lgtm.

[haraken, 2016-02-10 00:55:20]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-02-10 00:55:21]

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

[commit-bot: I haz the power, 2016-02-10 00:58:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609343002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609343002/80001

[commit-bot: I haz the power, 2016-02-10 02:49:47]

Description was changed from

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override
hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending
activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be
observed
by LifecycleObservers and notified when the observing context changes its
lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to
ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a
worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and
checking their
pending activities when the main thread posts a message to the worker. After
this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending
activities.
Given that the number of ScriptWrappables is larger than the number of
ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't
know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is
limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending
timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have
any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/
==========

to

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override
hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending
activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be
observed
by LifecycleObservers and notified when the observing context changes its
lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to
ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a
worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and
checking their
pending activities when the main thread posts a message to the worker. After
this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending
activities.
Given that the number of ScriptWrappables is larger than the number of
ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't
know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is
limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending
timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have
any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/
==========

[commit-bot: I haz the power, 2016-02-10 02:49:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-10 02:51:00]

Description was changed from

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override
hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending
activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be
observed
by LifecycleObservers and notified when the observing context changes its
lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to
ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a
worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and
checking their
pending activities when the main thread posts a message to the worker. After
this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending
activities.
Given that the number of ScriptWrappables is larger than the number of
ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't
know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is
limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending
timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have
any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/
==========

to

==========
Move hasPendingActivity from ActiveDOMObject to ScriptWrappable

Currently hasPendingActivity is a method of ActiveDOMObject (by accident),
so a lot of objects need to become ActiveDOMObject just to override
hasPendingActivity.
This is not nice in terms of both design and performance.

- In terms of design, ActiveDOMObject is an object whose lifetime is tied
with ExecutionContext. On the other hand, hasPendingActivity is a mechanism to
keep alive a wrapper while the underlying DOM object has some pending
activities.
These two are totally different notions.

- In terms of performance, ActiveDOMObjects are heavy because they must be
observed
by LifecycleObservers and notified when the observing context changes its
lifetime state.
It's not nice to increase the number of ActiveDOMObjects.

Given the above, this CL moves hasPendingActivity from ActiveDOMObject to
ScriptWrappable.

The most tricky part is how to detect the fact that all pending activities of a
worker are gone.
Before this CL, this was detected by scanning a list of ActiveDOMObjects and
checking their
pending activities when the main thread posts a message to the worker. After
this CL,
it is detected by scanning a list of ScriptWrappables and checking their pending
activities.
Given that the number of ScriptWrappables is larger than the number of
ActiveDOMObjects,
this CL will increase the overhead of scanning at every postMessage. I don't
know how large
the overhead is, but I hope that the number of ScriptWrappables in workers is
limited in
common cases and thus the overhead wouldn't be an issue.

This CL passes all tests in fast/workers, in particular the following tests:

- worker-messageport-gc.html (which checks that a worker that has a pending
timer task doesn't get garbage-collected)
- dedicated-worker-lifecycle.html (which checks that a worker that doesn't have
any pending activity is garbage-collected in a finite time period)

BUG=483722
TEST=fast/workers/

Committed: https://crrev.com/485c63ccd15b1e6efe6baf67010da6ea2db94e0d
Cr-Commit-Position: refs/heads/master@{#374584}
==========

[commit-bot: I haz the power, 2016-02-10 02:51:06]

Patchset 5 (id:??) landed as
https://crrev.com/485c63ccd15b1e6efe6baf67010da6ea2db94e0d
Cr-Commit-Position: refs/heads/master@{#374584}