Oilpan: mark ThreadAffinity as MainThreadOnly for Node or CSSValue inheriting classes.

Oilpan: mark ThreadAffinity as MainThreadOnly for Node or CSSValue inheriting classes.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168257

[kouhei (in TOK), 2014-02-21 06:49:07]

Proof-of-concept patch.

haraken: wdyt?

[haraken, 2014-02-21 06:51:07]

+oilpan-reviews@

Looks good but Mads should take a look.

https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h#ne...
Source/heap/ThreadState.h:96: struct DefaultThreadingTrait<T, false> {

Would you add a comment to mention that only Node and CSSValue use
MainThreadOnly?

[zerny-chromium, 2014-02-21 07:04:53]

dbc

https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h#ne...
Source/heap/ThreadState.h:93: template<typename T, bool derivesNodeOrCSSValue =
DerivesNodeOrCSSValue<T>::value > class DefaultThreadingTrait;
I think this can use:

  derivesNodeOrCSSValue = IsSubclass<T, Node>::value || IsSubClass<T,
CSSValue>::value

and then eliminate the DerivesNodeOrCSSValue template.

[kouhei (in TOK), 2014-02-21 07:08:17]

On 2014/02/21 07:04:53, zerny-chromium wrote:
> dbc
> 
> https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h
> File Source/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/171743004/diff/1/Source/heap/ThreadState.h#ne...
> Source/heap/ThreadState.h:93: template<typename T, bool derivesNodeOrCSSValue
=
> DerivesNodeOrCSSValue<T>::value > class DefaultThreadingTrait;
> I think this can use:
> 
>   derivesNodeOrCSSValue = IsSubclass<T, Node>::value || IsSubClass<T,
> CSSValue>::value
> 
> and then eliminate the DerivesNodeOrCSSValue template.

Good point!

[Mads Ager (chromium), 2014-02-21 09:22:28]

This change is the opposite of what I would expect. We should make CSSValue and
Node have default MainThreadOnly affinity because they are performance critical.
For everything else, we should use AnyThread affinity by default. Then we need
to explicitly make Document and other Nodes have default AnyThread affinity.

Are you sure this works? There are a lot of things in the list that are now
MainThreadOnly affinity with this change but are used from multiple threads?

[Mads Ager (chromium), 2014-02-21 09:23:08]

Hopefully Erik will have numbers from his mac today. Let's hope that we can go
for AnyThread affinity for anything without regressing...

[haraken, 2014-02-27 00:44:47]

The USED_FROM_MULTIPLE_THREAD macros have been causing real issues, so at the
moment I'd propose to land this CL until we can conclude that we can remove the
macros completely.

LGTM once the following Mads' comment is addressed:

> This change is the opposite of what I would expect. We should make CSSValue
and
> Node have default MainThreadOnly affinity because they are performance
critical.
> For everything else, we should use AnyThread affinity by default. Then we need
> to explicitly make Document and other Nodes have default AnyThread affinity.

[kouhei (in TOK), 2014-02-27 06:20:04]

On 2014/02/27 00:44:47, haraken wrote:
> The USED_FROM_MULTIPLE_THREAD macros have been causing real issues, so at the
> moment I'd propose to land this CL until we can conclude that we can remove
the
> macros completely.
> 
> LGTM once the following Mads' comment is addressed:
> 
> > This change is the opposite of what I would expect. We should make CSSValue
> and
> > Node have default MainThreadOnly affinity because they are performance
> critical.
> > For everything else, we should use AnyThread affinity by default. Then we
need
> > to explicitly make Document and other Nodes have default AnyThread affinity.

done.

[Mads Ager (chromium), 2014-02-27 10:01:26]

LGTM

[kouhei (in TOK), 2014-02-27 10:08:27]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-27 10:08:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/171743004/130001

[commit-bot: I haz the power, 2014-02-27 10:45:45]

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

[commit-bot: I haz the power, 2014-02-27 10:45:45]

Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[zerny-chromium, 2014-02-27 11:05:45]

FYI, the build is failing due to a class/struct mismatch.

https://codereview.chromium.org/171743004/diff/130001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/171743004/diff/130001/Source/heap/ThreadState...
Source/heap/ThreadState.h:77: template<typename T, bool derivesNodeOrCSSValue =
WTF::IsSubclass<T, Node>::value || WTF::IsSubclass<T, CSSValue>::value > class
DefaultThreadingTrait;
Error: class/struct mismatch.

[haraken, 2014-02-28 00:24:12]

On 2014/02/27 11:05:45, zerny-chromium wrote:
> FYI, the build is failing due to a class/struct mismatch.
> 
>
https://codereview.chromium.org/171743004/diff/130001/Source/heap/ThreadState.h
> File Source/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/171743004/diff/130001/Source/heap/ThreadState...
> Source/heap/ThreadState.h:77: template<typename T, bool derivesNodeOrCSSValue
=
> WTF::IsSubclass<T, Node>::value || WTF::IsSubclass<T, CSSValue>::value > class
> DefaultThreadingTrait;
> Error: class/struct mismatch.

Good point.

kouhei@: Would you fix that and land this CL? This is blocking a couple of CLs.
Thanks!

[haraken, 2014-02-28 01:25:38]

kouhei@ is ooo, so I'll take over the CL.

Since I cannot upload a CL to this issue (because this issue is owned by
kouhei@), so I uploaded a new CL here:
https://codereview.chromium.org/182733003/

[kouhei (in TOK), 2014-02-28 03:06:07]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-28 03:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/171743004/150001

[kouhei (in TOK), 2014-02-28 03:06:36]

On 2014/02/28 01:25:38, haraken wrote:
> kouhei@ is ooo, so I'll take over the CL.
> 
> Since I cannot upload a CL to this issue (because this issue is owned by
> kouhei@), so I uploaded a new CL here:
> https://codereview.chromium.org/182733003/

Uploaded the fix. checking cq

[commit-bot: I haz the power, 2014-02-28 04:13:33]

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

[commit-bot: I haz the power, 2014-02-28 04:13:34]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[kouhei (in TOK), 2014-02-28 04:18:13]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-28 04:18:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/171743004/150001

[commit-bot: I haz the power, 2014-02-28 05:47:41]

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

[commit-bot: I haz the power, 2014-02-28 05:47:42]

Retried try job too often on win_blink_rel for step(s) webkit_tests,
webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[haraken, 2014-02-28 07:25:01]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-28 07:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/171743004/150001

[haraken, 2014-02-28 07:35:09]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2014-02-28 07:50:01]

I hit a lot of issues about threading affinity when moving core/animations/ to
the heap, so I'll wait for this CL landed before working on core/animations :)

[kouhei (in TOK), 2014-02-28 07:53:12]

On 2014/02/28 07:50:01, haraken wrote:
> I hit a lot of issues about threading affinity when moving core/animations/ to
> the heap, so I'll wait for this CL landed before working on core/animations :)

Got it. Can you flip the CQ bit once that is resolved?

[haraken, 2014-02-28 08:07:27]

On 2014/02/28 07:53:12, kouhei wrote:
> On 2014/02/28 07:50:01, haraken wrote:
> > I hit a lot of issues about threading affinity when moving core/animations/
to
> > the heap, so I'll wait for this CL landed before working on core/animations
:)
> 
> Got it. Can you flip the CQ bit once that is resolved?

Sorry for not being clear.

I think we now have two compile errors:

- The first one will be fixed by https://codereview.chromium.org/182733004/.
However, the tree is almost closed today and thus we haven't yet landed the fix.

- The second one is the one you observe in win try bots in this CL. This failure
is different from the first one.

- After landing the two CLs (i.e., https://codereview.chromium.org/182733004/
and this CL), we can restart working on core/animations/, mediastream/ etc.

[kouhei (in TOK), 2014-03-03 00:33:53]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-03-03 00:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/171743004/170001

[commit-bot: I haz the power, 2014-03-03 03:14:51]

Change committed as 168257