[wrapper-tracing] Remove unnecessary barrier

[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240
Committed: https://crrev.com/8882ee9c09cbd64e21382197d4116a4a57d77394
Cr-Commit-Position: refs/heads/master@{#437870}

[Michael Lippautz, 2016-12-09 20:21:33]

Description was changed from

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240
==========

to

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240
==========

[Michael Lippautz, 2016-12-09 20:21:33]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, hlopko@google.com

[Michael Lippautz, 2016-12-09 20:25:24]

PTAL

After the actual fix in http://crrev.com/2567463003 we issue too many
unnecessary barriers, resulting in quite some overhead. We already cause ~5%
overhead on speedometer, which I measured is mainly due to duplicates in the
marking deque.

This is an attempt to reduce that overhead. We can watch the TraceWrappables bot
before enabling the flag again. 

I have more plans on reducing the duplicates on the marking deque by eagerly
marking the objects we put on there, similar to tricolor marking without the
extra bit.

[Michael Lippautz, 2016-12-09 20:33:27]

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

[commit-bot: I haz the power, 2016-12-09 20:34:21]

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, 2016-12-09 23:20:22]

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

[commit-bot: I haz the power, 2016-12-09 23:20:23]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-12 01:02:07]

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

https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:34: //   is
initially white.

To guarantee this fact, you need to guarantee that an incremental marking step
does not happen during a constructor, right?

class A : public TraceWrapperBase {
  A() : m_x(new X), // If this triggers an incremental marking step,
        m_y(new Y)  // The parent object (=A's object we're constructing now) is
marked as black at this point.
  { }
  Member<X> m_x;
  TraceWrapperMember<Y> m_y;
};

[Michael Lippautz, 2016-12-12 08:18:57]

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

https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:34: //   is
initially white.
On 2016/12/12 01:02:07, haraken wrote:
> 
> To guarantee this fact, you need to guarantee that an incremental marking step
> does not happen during a constructor, right?
> 
> class A : public TraceWrapperBase {
>   A() : m_x(new X), // If this triggers an incremental marking step,
>         m_y(new Y)  // The parent object (=A's object we're constructing now)
is
> marked as black at this point.
>   { }
>   Member<X> m_x;
>   TraceWrapperMember<Y> m_y;
> };

We (still) cannot handle this case :) We cannot mark during a ctor call as we
cannot tell the initialized from the uninitialized parts apart. 

The question that still remains is how we can enforce this on non-mixin cases.

[haraken, 2016-12-12 08:43:39]

On 2016/12/12 08:18:57, Michael Lippautz wrote:
>
https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h (right):
> 
>
https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:34: //   is
> initially white.
> On 2016/12/12 01:02:07, haraken wrote:
> > 
> > To guarantee this fact, you need to guarantee that an incremental marking
step
> > does not happen during a constructor, right?
> > 
> > class A : public TraceWrapperBase {
> >   A() : m_x(new X), // If this triggers an incremental marking step,
> >         m_y(new Y)  // The parent object (=A's object we're constructing
now)
> is
> > marked as black at this point.
> >   { }
> >   Member<X> m_x;
> >   TraceWrapperMember<Y> m_y;
> > };
> 
> We (still) cannot handle this case :) We cannot mark during a ctor call as we
> cannot tell the initialized from the uninitialized parts apart. 
> 
> The question that still remains is how we can enforce this on non-mixin cases.

Totally makes sense. LGTM.

[haraken, 2016-12-12 08:44:13]

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

https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:39: // Note that
support for black allocation would require a barrier here.

Can we add a dcheck to check if m_parent is a white object?

[Michael Lippautz, 2016-12-12 11:42:27]

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

https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:39: // Note that
support for black allocation would require a barrier here.
On 2016/12/12 08:44:13, haraken wrote:
> 
> Can we add a dcheck to check if m_parent is a white object?

Unfortunately not, as we use this type also as a wrapper type when we insert
into a container. We use the type there to avoid forgetting the barrier.

E.g.
m_vector.append(TraceWrapperMember<SomeType>(this, newValue);
                ^^^^^^^^^^^^^^^^^^
                'this' might already be black here

We delay the barrier until we actually append into the backingstore at which
point the copy ctor will fire the barrier.

[haraken, 2016-12-12 11:44:02]

On 2016/12/12 11:42:27, Michael Lippautz wrote:
>
https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h (right):
> 
>
https://codereview.chromium.org/2564153002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:39: // Note
that
> support for black allocation would require a barrier here.
> On 2016/12/12 08:44:13, haraken wrote:
> > 
> > Can we add a dcheck to check if m_parent is a white object?
> 
> Unfortunately not, as we use this type also as a wrapper type when we insert
> into a container. We use the type there to avoid forgetting the barrier.
> 
> E.g.
> m_vector.append(TraceWrapperMember<SomeType>(this, newValue);
>                 ^^^^^^^^^^^^^^^^^^
>                 'this' might already be black here
> 
> We delay the barrier until we actually append into the backingstore at which
> point the copy ctor will fire the barrier.

Makes sense.

[Michael Lippautz, 2016-12-12 12:39:39]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2016-12-12 12:39:52]

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, 2016-12-12 14:41:46]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481546379081550, "parent_rev":
"0cda7322744067dec32fcffd45895bcc27892360", "commit_rev":
"11619fcdb65b2188aa76ef91af81c4db4b155091"}

[commit-bot: I haz the power, 2016-12-12 14:42:13]

Description was changed from

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240
==========

to

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240

Review-Url: https://codereview.chromium.org/2564153002
==========

[commit-bot: I haz the power, 2016-12-12 14:42:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-12 15:12:12]

Description was changed from

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240

Review-Url: https://codereview.chromium.org/2564153002
==========

to

==========
[wrapper-tracing] Remove unnecessary barrier

We don't require a write barrier here as TraceWrapperMember is used for the
following scenarios:
- Initial initialization: The write barrier will not fire as the parent is
   initially white.
- Wrapping when inserting into a container: The write barrier will fire upon
  establishing the move into the container.
- Assignment to a field: The regular assignment operator will fire the write
  barrier.

Note that support for black allocation would require a barrier here.

BUG=chromium:468240

Committed: https://crrev.com/8882ee9c09cbd64e21382197d4116a4a57d77394
Cr-Commit-Position: refs/heads/master@{#437870}
==========

[commit-bot: I haz the power, 2016-12-12 15:12:13]

Patchset 1 (id:??) landed as
https://crrev.com/8882ee9c09cbd64e21382197d4116a4a57d77394
Cr-Commit-Position: refs/heads/master@{#437870}