Bindings: Manage the main world separately from other worlds

Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression on a perf test (see issue 704778)
probably because of changing the hashmap traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG=697622, 697629, 704778
Review-Url: https://codereview.chromium.org/2777763003
Cr-Commit-Position: refs/heads/master@{#460294}
Committed: https://chromium.googlesource.com/chromium/src/+/3e5d3fc45d06833189606b0ad750b9dee95ef50a

[nhiroki, 2017-03-27 01:25:13]

Description was changed from

==========
Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression probably because of changing the hashmap
traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG=697622, 697629, 704778
==========

to

==========
Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression on a perf test (see issue 704778)
probably because of changing the hashmap traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG=697622, 697629, 704778
==========

[nhiroki, 2017-03-27 01:25:20]

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

[commit-bot: I haz the power, 2017-03-27 01:25:32]

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

[nhiroki, 2017-03-27 01:26:42]

nhiroki@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[nhiroki, 2017-03-27 01:26:42]

PTAL, thanks!

[haraken, 2017-03-27 01:40:15]

Why do you think this will fix the perf regression? This CL won't change the
overhead to look up the main world (i.e., the overhead to call mainWorld()),
right?

[nhiroki, 2017-03-27 02:08:23]

On 2017/03/27 01:40:15, haraken wrote:
> Why do you think this will fix the perf regression? This CL won't change the
> overhead to look up the main world (i.e., the overhead to call mainWorld()),
> right?

Yeah, the overhead to look up should not be changed. I think this CL could
reduce...

1) the overhead to create a main world because it no longer needs to be pushed
into the world map, and
2) the overhead to look up other worlds because the hashtraits is reverted to
the default algorithm.

(I'm not sure what the perf test does and how much this revert will affect, but
the regression would be real according to the report)

[nhiroki, 2017-03-27 02:09:28]

On 2017/03/27 02:08:23, nhiroki wrote:
> On 2017/03/27 01:40:15, haraken wrote:
> > Why do you think this will fix the perf regression? This CL won't change the
> > overhead to look up the main world (i.e., the overhead to call mainWorld()),
> > right?
> 
> Yeah, the overhead to look up should not be changed. I think this CL could
> reduce...

^^^ the overhead to look up *the main world* should not be changed

> 1) the overhead to create a main world because it no longer needs to be pushed
> into the world map, and
> 2) the overhead to look up other worlds because the hashtraits is reverted to
> the default algorithm.
> 
> (I'm not sure what the perf test does and how much this revert will affect,
but
> the regression would be real according to the report)

[haraken, 2017-03-27 02:16:50]

On 2017/03/27 02:09:28, nhiroki wrote:
> On 2017/03/27 02:08:23, nhiroki wrote:
> > On 2017/03/27 01:40:15, haraken wrote:
> > > Why do you think this will fix the perf regression? This CL won't change
the
> > > overhead to look up the main world (i.e., the overhead to call
mainWorld()),
> > > right?
> > 
> > Yeah, the overhead to look up should not be changed. I think this CL could
> > reduce...
> 
> ^^^ the overhead to look up *the main world* should not be changed
> 
> > 1) the overhead to create a main world because it no longer needs to be
pushed
> > into the world map, and
> > 2) the overhead to look up other worlds because the hashtraits is reverted
to
> > the default algorithm.
> > 
> > (I'm not sure what the perf test does and how much this revert will affect,
> but
> > the regression would be real according to the report)

Yeah, I don't think the perf test is looking up other worlds...

[commit-bot: I haz the power, 2017-03-27 02:43:07]

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

[commit-bot: I haz the power, 2017-03-27 02:43:07]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-03-27 05:22:10]

Just in case, I'm now running another bisect job with a wider division range.

[nhiroki, 2017-03-28 01:48:57]

On 2017/03/27 05:22:10, nhiroki wrote:
> Just in case, I'm now running another bisect job with a wider division range.

Hm... the bisect job pointed at my change again :(

We would have 2 options here: 1) speculatively landing this CL and monitor the
graph, or 2) reverting the entire culprit CL and then trying to land a harmless
part of the CL again. Any thoughts?

On 2017/03/27 02:16:50, haraken wrote:
> On 2017/03/27 02:09:28, nhiroki wrote:
> > On 2017/03/27 02:08:23, nhiroki wrote:
> > > On 2017/03/27 01:40:15, haraken wrote:
> > > > Why do you think this will fix the perf regression? This CL won't change
> the
> > > > overhead to look up the main world (i.e., the overhead to call
> mainWorld()),
> > > > right?
> > > 
> > > Yeah, the overhead to look up should not be changed. I think this CL could
> > > reduce...
> > 
> > ^^^ the overhead to look up *the main world* should not be changed
> > 
> > > 1) the overhead to create a main world because it no longer needs to be
> pushed
> > > into the world map, and
> > > 2) the overhead to look up other worlds because the hashtraits is reverted
> to
> > > the default algorithm.
> > > 
> > > (I'm not sure what the perf test does and how much this revert will
affect,
> > but
> > > the regression would be real according to the report)
> 
> Yeah, I don't think the perf test is looking up other worlds...

How about WorldType::GarbageCollector? Can it be used in the test?

[haraken, 2017-03-28 01:50:56]

On 2017/03/28 01:48:57, nhiroki wrote:
> On 2017/03/27 05:22:10, nhiroki wrote:
> > Just in case, I'm now running another bisect job with a wider division
range.
> 
> Hm... the bisect job pointed at my change again :(
> 
> We would have 2 options here: 1) speculatively landing this CL and monitor the
> graph, or 2) reverting the entire culprit CL and then trying to land a
harmless
> part of the CL again. Any thoughts?
> 
> On 2017/03/27 02:16:50, haraken wrote:
> > On 2017/03/27 02:09:28, nhiroki wrote:
> > > On 2017/03/27 02:08:23, nhiroki wrote:
> > > > On 2017/03/27 01:40:15, haraken wrote:
> > > > > Why do you think this will fix the perf regression? This CL won't
change
> > the
> > > > > overhead to look up the main world (i.e., the overhead to call
> > mainWorld()),
> > > > > right?
> > > > 
> > > > Yeah, the overhead to look up should not be changed. I think this CL
could
> > > > reduce...
> > > 
> > > ^^^ the overhead to look up *the main world* should not be changed
> > > 
> > > > 1) the overhead to create a main world because it no longer needs to be
> > pushed
> > > > into the world map, and
> > > > 2) the overhead to look up other worlds because the hashtraits is
reverted
> > to
> > > > the default algorithm.
> > > > 
> > > > (I'm not sure what the perf test does and how much this revert will
> affect,
> > > but
> > > > the regression would be real according to the report)
> > 
> > Yeah, I don't think the perf test is looking up other worlds...
> 
> How about WorldType::GarbageCollector? Can it be used in the test?

LGTM to try this CL. Would you add a comment on why you don't want to put
m_mainWorld to the world map?

[Yuki, 2017-03-28 04:13:25]

LGTM.

https://codereview.chromium.org/2777763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2777763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:146: if
(isMainThread() && s_wasMainWorldCreatedOnMainThread)
Do we really need this flag?
IIUC, it's unlikely that we don't have the main world.  Even if not yet, sooner
or later, the main world will be created, I think.

Then, I'd like to do the same thing before crrev.com/2753013003 .

  if (isMainThread())
    worlds.push_back(&mainWorld());

[nhiroki, 2017-03-29 02:26:48]

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

[commit-bot: I haz the power, 2017-03-29 02:27:31]

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

[nhiroki, 2017-03-29 03:33:02]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2017-03-29 03:34:03]

Thank you. Updated.

https://codereview.chromium.org/2777763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2777763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:146: if
(isMainThread() && s_wasMainWorldCreatedOnMainThread)
On 2017/03/28 04:13:25, Yuki wrote:
> Do we really need this flag?
> IIUC, it's unlikely that we don't have the main world.  Even if not yet,
sooner
> or later, the main world will be created, I think.
> 
> Then, I'd like to do the same thing before crrev.com/2753013003 .
> 
>   if (isMainThread())
>     worlds.push_back(&mainWorld());

Done.

[nhiroki, 2017-03-29 03:34:15]

On 2017/03/28 01:50:56, haraken wrote:
> On 2017/03/28 01:48:57, nhiroki wrote:
> > On 2017/03/27 05:22:10, nhiroki wrote:
> > > Just in case, I'm now running another bisect job with a wider division
> range.
> > 
> > Hm... the bisect job pointed at my change again :(
> > 
> > We would have 2 options here: 1) speculatively landing this CL and monitor
the
> > graph, or 2) reverting the entire culprit CL and then trying to land a
> harmless
> > part of the CL again. Any thoughts?
> > 
> > On 2017/03/27 02:16:50, haraken wrote:
> > > On 2017/03/27 02:09:28, nhiroki wrote:
> > > > On 2017/03/27 02:08:23, nhiroki wrote:
> > > > > On 2017/03/27 01:40:15, haraken wrote:
> > > > > > Why do you think this will fix the perf regression? This CL won't
> change
> > > the
> > > > > > overhead to look up the main world (i.e., the overhead to call
> > > mainWorld()),
> > > > > > right?
> > > > > 
> > > > > Yeah, the overhead to look up should not be changed. I think this CL
> could
> > > > > reduce...
> > > > 
> > > > ^^^ the overhead to look up *the main world* should not be changed
> > > > 
> > > > > 1) the overhead to create a main world because it no longer needs to
be
> > > pushed
> > > > > into the world map, and
> > > > > 2) the overhead to look up other worlds because the hashtraits is
> reverted
> > > to
> > > > > the default algorithm.
> > > > > 
> > > > > (I'm not sure what the perf test does and how much this revert will
> > affect,
> > > > but
> > > > > the regression would be real according to the report)
> > > 
> > > Yeah, I don't think the perf test is looking up other worlds...
> > 
> > How about WorldType::GarbageCollector? Can it be used in the test?
> 
> LGTM to try this CL. Would you add a comment on why you don't want to put
> m_mainWorld to the world map?

Added the comment.

[nhiroki, 2017-03-29 03:34:19]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-03-29 03:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2777763003/#ps40001
(title: "address review comments")

[commit-bot: I haz the power, 2017-03-29 03:34:58]

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

[commit-bot: I haz the power, 2017-03-29 05:35:53]

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

[commit-bot: I haz the power, 2017-03-29 05:35:54]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[nhiroki, 2017-03-29 05:42:13]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2017-03-29 05:42:33]

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

[commit-bot: I haz the power, 2017-03-29 05:53:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490766133442540,
"parent_rev": "f64ac5cb273e3abab3564f1a5997b3da164ab9ff", "commit_rev":
"3e5d3fc45d06833189606b0ad750b9dee95ef50a"}

[commit-bot: I haz the power, 2017-03-29 05:54:02]

Description was changed from

==========
Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression on a perf test (see issue 704778)
probably because of changing the hashmap traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG=697622, 697629, 704778
==========

to

==========
Bindings: Manage the main world separately from other worlds

This CL speculatively reverts a part of the CL[1].

The CL[1] pushed the main world into the world map for cleanup, but that CL
seems to cause a performance regression on a perf test (see issue 704778)
probably because of changing the hashmap traits and/or the world map management.

[1] https://codereview.chromium.org/2753013003/

BUG=697622, 697629, 704778

Review-Url: https://codereview.chromium.org/2777763003
Cr-Commit-Position: refs/heads/master@{#460294}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e5d3fc45d06833189606b0ad750...
==========

[commit-bot: I haz the power, 2017-03-29 05:54:05]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3e5d3fc45d06833189606b0ad750...