[wrapper-tracing] Emite write barrier upon associating a wrapper

[wrapper-tracing] Emite write barrier upon associating a wrapper

Whenever we assign a reference that should be traced into V8 we need to
emit a write barrier. We automatically emit write barriers for
TraceWrapperv8Reference. However, we still use a regular (weak)
Persistent in ScriptWrappable, so we need to emit the barrier there
manually.

A follow up should convert the reference in ScriptWrappable to
TraceWrapperv8Reference.

BUG=chromium:702490, chromium:701601, chromium:468240
Review-Url: https://codereview.chromium.org/2759243002
Cr-Commit-Position: refs/heads/master@{#458357}
Committed: https://chromium.googlesource.com/chromium/src/+/a2011780ffefd6f9c120b80651015b3f6646244b

[Michael Lippautz, 2017-03-20 19:28:41]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[Michael Lippautz, 2017-03-20 19:28:42]

jochen: fyi, this is what we were talking about
haraken: ptal, this needs to be back merged to M57 and M58.

Next step is to convert SW to use TraceWrapperV8Reference as this one is
automatically emitting the barrier.

The reason we didn't catch this is that our verifier operates in Blink land, and
the tests operate in either Blink or V8, but not in the combination of both. I
will add a proper intercepting test when I convert SW to use
TraceWrapperV8Reference. For now I just want this to be out and backmerged asap.

[Michael Lippautz, 2017-03-20 19:28:48]

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

[commit-bot: I haz the power, 2017-03-20 19:29:07]

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-03-20 20:47:31]

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

[commit-bot: I haz the power, 2017-03-20 20:47:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jochen (gone - plz use gerrit), 2017-03-20 20:59:40]

https://codereview.chromium.org/2759243002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/2759243002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:129:
ScriptWrappableVisitor::writeBarrier(this, &m_mainWorldWrapper);
"this" is not necessarily pointing to the beginning of the object...

[Michael Lippautz, 2017-03-20 21:04:22]

https://codereview.chromium.org/2759243002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/2759243002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:129:
ScriptWrappableVisitor::writeBarrier(this, &m_mainWorldWrapper);
On 2017/03/20 20:59:39, jochen wrote:
> "this" is not necessarily pointing to the beginning of the object...

do you have an example? I think it was the assumption that SW always has 'this'
pointing to the object start. as far as I can see we only have GarbageCollected
and GarbageCollectedFinalized in front of SW in the inheritance hierarchy.

[jochen (gone - plz use gerrit), 2017-03-20 21:11:34]

SVGPointListTearOff for example

[Michael Lippautz, 2017-03-20 21:15:14]

On 2017/03/20 21:11:34, jochen wrote:
> SVGPointListTearOff for example

Interesting... the use of SW as sort of mixin is new for me. I always thought it
is on the main inheritance hierarchy. In that case I need to think about this a
bit longer, even though this is already broken on stable.

[jochen (gone - plz use gerrit), 2017-03-20 21:23:27]

just mark the v8 object - black allocation ftw

[Michael Lippautz, 2017-03-20 21:50:55]

On 2017/03/20 21:23:27, jochen wrote:
> just mark the v8 object - black allocation ftw

Our marking assumes that we are either in incremental marking or in a full GC.
If possible I'd like to fix this in the inheritance hierarchy and avoid
backmering a patch in V8 first and then Chrome. Lets talk tomorrow! 

https://cs.chromium.org/chromium/src/v8/src/heap/heap.cc?sq=package:chromium&...

[haraken, 2017-03-20 23:48:38]

LGTM

On 2017/03/20 21:15:14, Michael Lippautz wrote:
> On 2017/03/20 21:11:34, jochen wrote:
> > SVGPointListTearOff for example
> 
> Interesting... the use of SW as sort of mixin is new for me. I always thought
it
> is on the main inheritance hierarchy. In that case I need to think about this
a
> bit longer, even though this is already broken on stable.

I think it should be already guaranteed that SW's beginning address is equal to
the beginning address of the object. SW is not the left-most class of
SVGPointListTearOff but SW's beginning address is equal to the beginning address
of the object. (However, it would be nicer to make SW the left-most class for
clarity.)

If the assumption is broken, you'll hit a dcheck in checkHeader() when marking
the SW.

[jochen (gone - plz use gerrit), 2017-03-21 05:36:35]

On 2017/03/20 at 23:48:38, haraken wrote:
> LGTM
> 
> On 2017/03/20 21:15:14, Michael Lippautz wrote:
> > On 2017/03/20 21:11:34, jochen wrote:
> > > SVGPointListTearOff for example
> > 
> > Interesting... the use of SW as sort of mixin is new for me. I always
thought it
> > is on the main inheritance hierarchy. In that case I need to think about
this a
> > bit longer, even though this is already broken on stable.
> 
> I think it should be already guaranteed that SW's beginning address is equal
to the beginning address of the object. SW is not the left-most class of
SVGPointListTearOff but SW's beginning address is equal to the beginning address
of the object. (However, it would be nicer to make SW the left-most class for
clarity.)
> 

nah, SVGPointListTearOff inherits from SVGListPropertyTearOffHelper inherits
from SVGPropertyTearOff which has a member.

> If the assumption is broken, you'll hit a dcheck in checkHeader() when marking
the SW.

... and so the bots are all read :)

[Michael Lippautz, 2017-03-21 07:46:16]

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

[commit-bot: I haz the power, 2017-03-21 07:46:38]

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

[Michael Lippautz, 2017-03-21 07:47:36]

On 2017/03/21 05:36:35, jochen wrote:
> On 2017/03/20 at 23:48:38, haraken wrote:
> > LGTM
> > 
> > On 2017/03/20 21:15:14, Michael Lippautz wrote:
> > > On 2017/03/20 21:11:34, jochen wrote:
> > > > SVGPointListTearOff for example
> > > 
> > > Interesting... the use of SW as sort of mixin is new for me. I always
> thought it
> > > is on the main inheritance hierarchy. In that case I need to think about
> this a
> > > bit longer, even though this is already broken on stable.
> > 
> > I think it should be already guaranteed that SW's beginning address is equal
> to the beginning address of the object. SW is not the left-most class of
> SVGPointListTearOff but SW's beginning address is equal to the beginning
address
> of the object. (However, it would be nicer to make SW the left-most class for
> clarity.)
> > 
> 
> nah, SVGPointListTearOff inherits from SVGListPropertyTearOffHelper inherits
> from SVGPropertyTearOff which has a member.
> 
> > If the assumption is broken, you'll hit a dcheck in checkHeader() when
marking
> the SW.
> 
> ... and so the bots are all read :)

I switched to unconditional marking without checking a back pointer for SW.
Obviously, we still check whether tracing is on. This is probably safer to
backmerge then changing the inheritance hierarchy for SW.

[haraken, 2017-03-21 07:50:14]

On 2017/03/21 07:47:36, Michael Lippautz wrote:
> On 2017/03/21 05:36:35, jochen wrote:
> > On 2017/03/20 at 23:48:38, haraken wrote:
> > > LGTM
> > > 
> > > On 2017/03/20 21:15:14, Michael Lippautz wrote:
> > > > On 2017/03/20 21:11:34, jochen wrote:
> > > > > SVGPointListTearOff for example
> > > > 
> > > > Interesting... the use of SW as sort of mixin is new for me. I always
> > thought it
> > > > is on the main inheritance hierarchy. In that case I need to think about
> > this a
> > > > bit longer, even though this is already broken on stable.
> > > 
> > > I think it should be already guaranteed that SW's beginning address is
equal
> > to the beginning address of the object. SW is not the left-most class of
> > SVGPointListTearOff but SW's beginning address is equal to the beginning
> address
> > of the object. (However, it would be nicer to make SW the left-most class
for
> > clarity.)
> > > 
> > 
> > nah, SVGPointListTearOff inherits from SVGListPropertyTearOffHelper inherits
> > from SVGPropertyTearOff which has a member.
> > 
> > > If the assumption is broken, you'll hit a dcheck in checkHeader() when
> marking
> > the SW.
> > 
> > ... and so the bots are all read :)
> 
> I switched to unconditional marking without checking a back pointer for SW.
> Obviously, we still check whether tracing is on. This is probably safer to
> backmerge then changing the inheritance hierarchy for SW.

Does it mean that we push all ScriptWrappables to the marking stack when they're
created? Won't it add a lot of overhead to it?

Also how hard would it be to make sure that ScriptWrappables are the left-most
classes?

[jochen (gone - plz use gerrit), 2017-03-21 07:56:25]

this lgtm

as proposed off-line, I'd fix the inheritance in a separate CL. Just add a
checkHeader() call to the ScriptWrappable constructor - that should catch all
the places :)

[Michael Lippautz, 2017-03-21 07:57:52]

On 2017/03/21 07:50:14, haraken wrote:
> Does it mean that we push all ScriptWrappables to the marking stack when
they're
> created? Won't it add a lot of overhead to it?
> 

When we are in tracing yes. This is overhead if we assume that we associate
wrappers to dead wrappables. During GC you usually assume the opposite though:
Whenever we assign a wrapper to a wrappabler we somehow hold a reference to the
wrappable, making it a live object. This is a actually a valid GC optimization
in some cases. I'd still like to do a proper barrier once the inheritance is
fixed.

> Also how hard would it be to make sure that ScriptWrappables are the left-most
> classes?

I will try to do that after this commit on master but have a small change for
backmerging.

[haraken, 2017-03-21 07:58:30]

On 2017/03/21 07:57:52, Michael Lippautz wrote:
> On 2017/03/21 07:50:14, haraken wrote:
> > Does it mean that we push all ScriptWrappables to the marking stack when
> they're
> > created? Won't it add a lot of overhead to it?
> > 
> 
> When we are in tracing yes. This is overhead if we assume that we associate
> wrappers to dead wrappables. During GC you usually assume the opposite though:
> Whenever we assign a wrapper to a wrappabler we somehow hold a reference to
the
> wrappable, making it a live object. This is a actually a valid GC optimization
> in some cases. I'd still like to do a proper barrier once the inheritance is
> fixed.
> 
> > Also how hard would it be to make sure that ScriptWrappables are the
left-most
> > classes?
> 
> I will try to do that after this commit on master but have a small change for
> backmerging.

LGTM

[Michael Lippautz, 2017-03-21 08:20:04]

Description was changed from

==========
[wrapper-tracing] Emite write barrier upon associating a wrapper

Whenever we assign a reference that should be traced into V8 we need to
emit a write barrier. We automatically emit write barriers for
TraceWrapperv8Reference. However, we still use a regular (weak)
Persistent in ScriptWrappable, so we need to emit the barrier there
manually.

A follow up should convert the reference in ScriptWrappable to
TraceWrapperv8Reference.

BUG=chromium:702490, chromium:468240
==========

to

==========
[wrapper-tracing] Emite write barrier upon associating a wrapper

Whenever we assign a reference that should be traced into V8 we need to
emit a write barrier. We automatically emit write barriers for
TraceWrapperv8Reference. However, we still use a regular (weak)
Persistent in ScriptWrappable, so we need to emit the barrier there
manually.

A follow up should convert the reference in ScriptWrappable to
TraceWrapperv8Reference.

BUG=chromium:702490, chromium:701601, chromium:468240
==========

[commit-bot: I haz the power, 2017-03-21 09:23:04]

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

[commit-bot: I haz the power, 2017-03-21 09:23:04]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-03-21 09:23:23]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2017-03-21 09:23:35]

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-21 09:27:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490088203562850,
"parent_rev": "8c6a07bf80bd57cb296211794dc1f0d0d71a28f9", "commit_rev":
"a2011780ffefd6f9c120b80651015b3f6646244b"}

[commit-bot: I haz the power, 2017-03-21 09:27:38]

Description was changed from

==========
[wrapper-tracing] Emite write barrier upon associating a wrapper

Whenever we assign a reference that should be traced into V8 we need to
emit a write barrier. We automatically emit write barriers for
TraceWrapperv8Reference. However, we still use a regular (weak)
Persistent in ScriptWrappable, so we need to emit the barrier there
manually.

A follow up should convert the reference in ScriptWrappable to
TraceWrapperv8Reference.

BUG=chromium:702490, chromium:701601, chromium:468240
==========

to

==========
[wrapper-tracing] Emite write barrier upon associating a wrapper

Whenever we assign a reference that should be traced into V8 we need to
emit a write barrier. We automatically emit write barriers for
TraceWrapperv8Reference. However, we still use a regular (weak)
Persistent in ScriptWrappable, so we need to emit the barrier there
manually.

A follow up should convert the reference in ScriptWrappable to
TraceWrapperv8Reference.

BUG=chromium:702490, chromium:701601, chromium:468240

Review-Url: https://codereview.chromium.org/2759243002
Cr-Commit-Position: refs/heads/master@{#458357}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2011780ffefd6f9c120b8065101...
==========

[commit-bot: I haz the power, 2017-03-21 09:27:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a2011780ffefd6f9c120b8065101...