Devirtualize Visitor and remove inline visitor specialization.

Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used for that, i.e., to perform GC
marking, so the flexibility it provides by way of overridable virtual
methods, is unused. And it slows down the GC marking phase, something
the specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor and nothing else. Besides removing code complexity,
this also allows the removal of InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers show a ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android(ARM) official build, content shell) is 180k less.

R=
BUG=683019
Review-Url: https://codereview.chromium.org/2652923002
Cr-Commit-Position: refs/heads/master@{#445993}
Committed: https://chromium.googlesource.com/chromium/src/+/66ac8d1a05c988aa07a1da9e1613c83ccdc304ad

[sof, 2017-01-24 08:55:58]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 08:56:14]

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

[sof, 2017-01-24 09:09:11]

Description was changed from

==========
WIP: devirtualize Visitor.

R=
BUG=
==========

to

==========
Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used to perform GC marking, so
the flexibility it provides by way of overridable virtual methods,
is unused. And it slows down the GC marking phase, something the
specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor, and nothing else. Besides removing code complexity,
this also allows the removal of the InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers shows ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android arm official build, content shell) is 180k less.

R=
BUG=683019
==========

[sof, 2017-01-24 09:11:09]

Description was changed from

==========
Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used to perform GC marking, so
the flexibility it provides by way of overridable virtual methods,
is unused. And it slows down the GC marking phase, something the
specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor, and nothing else. Besides removing code complexity,
this also allows the removal of the InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers shows ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android arm official build, content shell) is 180k less.

R=
BUG=683019
==========

to

==========
Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used for that, i.e., to perform GC
marking, so the flexibility it provides by way of overridable virtual
methods, is unused. And it slows down the GC marking phase, something
the specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor and nothing else. Besides removing code complexity,
this also allows the removal of InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers show a ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android(ARM) official build, content shell) is 180k less.

R=
BUG=683019
==========

[sof, 2017-01-24 09:13:25]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2017-01-24 09:13:26]

please take a look.

I think it is acceptable to just remove the two HeapTest unit tests, given what
they tested (ref-counted-GCed objects + TraceIfNeeded<> sanity checking.)

[commit-bot: I haz the power, 2017-01-24 10:42:24]

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

[commit-bot: I haz the power, 2017-01-24 10:42:24]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-01-25 06:14:07]

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

[haraken, 2017-01-25 06:14:08]

LGTM, great simplification!

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:707: #include
"platform/heap/VisitorImpl.h"

It looks weird to include a header file at the end of Heap.h...?

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.h (right):

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.h:363: using
MarkedPointerCallback = bool (*)(HeapObjectHeader*);

MarkedPointerCallbackForTesting ?

[sof, 2017-01-25 06:25:24]

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:707: #include
"platform/heap/VisitorImpl.h"
On 2017/01/25 06:14:08, haraken wrote:
> 
> It looks weird to include a header file at the end of Heap.h...?

The inlines depend on definitions above, so anywhere else isn't possible. And
we've got a number of places where '#include "Heap.h"' is assumed to get you a
complete definition of "Visitor", including inlines, so that's why it ended up
here.

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.h (right):

https://codereview.chromium.org/2652923002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.h:363: using
MarkedPointerCallback = bool (*)(HeapObjectHeader*);
On 2017/01/25 06:14:08, haraken wrote:
> 
> MarkedPointerCallbackForTesting ?

Done.

[sof, 2017-01-25 06:25:37]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 06:25:46]

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

[commit-bot: I haz the power, 2017-01-25 06:37:40]

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

[commit-bot: I haz the power, 2017-01-25 06:37:41]

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

[sof, 2017-01-25 07:47:33]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-01-25 07:47:33]

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

[commit-bot: I haz the power, 2017-01-25 07:47:54]

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

[sof, 2017-01-25 07:59:42]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-01-25 07:59:43]

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

[commit-bot: I haz the power, 2017-01-25 08:00:01]

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

[sof, 2017-01-25 08:03:19]

Removing the DEFINE_TRACE() et al macros from the codebase is an option if this
one sticks.

[commit-bot: I haz the power, 2017-01-25 08:12:09]

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

[commit-bot: I haz the power, 2017-01-25 08:12:11]

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

[sof, 2017-01-25 08:25:20]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-01-25 08:25:21]

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

[commit-bot: I haz the power, 2017-01-25 08:25:43]

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

[haraken, 2017-01-25 08:55:39]

On 2017/01/25 08:03:19, sof wrote:
> Removing the DEFINE_TRACE() et al macros from the codebase is an option if
this
> one sticks.

Yeah, the macros are no longer needed, so that's an option.

(I don't have any strong opinion since developers will already be used to the
macros.)

[commit-bot: I haz the power, 2017-01-25 10:20:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485332720912800,
"parent_rev": "7ca1c566e7af0996087c949dae38bfc7af908a37", "commit_rev":
"66ac8d1a05c988aa07a1da9e1613c83ccdc304ad"}

[commit-bot: I haz the power, 2017-01-25 10:21:06]

Description was changed from

==========
Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used for that, i.e., to perform GC
marking, so the flexibility it provides by way of overridable virtual
methods, is unused. And it slows down the GC marking phase, something
the specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor and nothing else. Besides removing code complexity,
this also allows the removal of InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers show a ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android(ARM) official build, content shell) is 180k less.

R=
BUG=683019
==========

to

==========
Devirtualize Visitor and remove inline visitor specialization.

The Blink GC infrastructure requires its managed objects to provide
a "trace()" method which will visit all the heap references it keeps
into the Blink GC heap, by calling the "trace()" method on each of
these via an incoming |visitor| argument (a Visitor.)

The Visitor interface is really only used for that, i.e., to perform GC
marking, so the flexibility it provides by way of overridable virtual
methods, is unused. And it slows down the GC marking phase, something
the specialized "inline visitor" (InlineGlobalMarkingVisitor) demonstrated,
which devirtualized the mark() method, with noticable improvements to
overall GC marking times.

Given that and Visitor's use, devirtualize Visitor entirely and make
it a GC marking visitor and nothing else. Besides removing code complexity,
this also allows the removal of InlineGlobalMarkingVisitor along with
all the specialized trace() implementation methods that we emit for each
Blink GC managed object.

Performance numbers show a ~10% improvement on marking times for
oilpan_gc_times.{tough_animation_cases, blink_perf_stress}; code size
(Android(ARM) official build, content shell) is 180k less.

R=
BUG=683019

Review-Url: https://codereview.chromium.org/2652923002
Cr-Commit-Position: refs/heads/master@{#445993}
Committed:
https://chromium.googlesource.com/chromium/src/+/66ac8d1a05c988aa07a1da9e1613...
==========

[commit-bot: I haz the power, 2017-01-25 10:21:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/66ac8d1a05c988aa07a1da9e1613...