Avoid calling markAncestorsWithChildNeedsStyleRecalc() constantly in childrenChanged

Avoid calling markAncestorsWithChildNeedsStyleRecalc() constantly in childrenChanged

This shows up in the Shadow DOM benchmark where childrenChanged is 2% of
the profile because we're calling into inActiveDocument() which is not
inline and then doing markAncestorsWithChildNeedsStyleRecalc() which is
also not inline without checking if the ancestor marking is actually needed.

markAncestorsWithChildNeedsStyleRecalc() also blindly calls
document().scheduleRenderTreeUpdate() which is not inline and does a bunch
of work.

This patch fixes the checks in ::childrenChanged to check the childNeedsStyleRecalc()
bit before calling markAncestorsWithChildNeedsStyleRecalc. This is equivalent to
checking inActiveDocument() as well since all nodes not in the tree have this bit set,
and all nodes in an inactive document will also have this bit set, so we'd never call
into markAncestorsWithChildNeedsStyleRecalc() in those trees.

It also adds an early  out so we don't call into scheduleRenderTreeUpdate() so often
when invalidating  style.

R=eseidel@chromium.org,abarth@chromium.org
BUG=357087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170705

[eseidel, 2014-03-31 21:01:33]

https://codereview.chromium.org/219993002/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/219993002/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:747: if (document().hasPendingStyleRecalc())
Why this additional check?  Does document().scheduleRenderTreeUpdate not already
check this?

[esprehn, 2014-03-31 21:04:08]

On 2014/03/31 21:01:33, eseidel wrote:
> https://codereview.chromium.org/219993002/diff/1/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/219993002/diff/1/Source/core/dom/Node.cpp#new...
> Source/core/dom/Node.cpp:747: if (document().hasPendingStyleRecalc())
> Why this additional check?  Does document().scheduleRenderTreeUpdate not
already
> check this?

Yes, but it's not inline and neither is the function that does the check. So you
pay 2 function calls for every style invalidation even after the recalc is
scheduled.

[eseidel, 2014-03-31 22:22:52]

OK.  My only concern is the lifecycle enum and the Document/RnederView's bool
being out of sync.  This came up with the parser spin: crbug.com/288265.

lgtm.

[esprehn, 2014-03-31 22:29:13]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:29:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/1

[commit-bot: I haz the power, 2014-03-31 22:47:00]

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

[commit-bot: I haz the power, 2014-03-31 22:47:00]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[esprehn, 2014-04-01 00:18:48]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/1

[commit-bot: I haz the power, 2014-04-01 00:35:59]

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

[commit-bot: I haz the power, 2014-04-01 00:35:59]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel
  tryserver.blink on mac_blink_rel

[esprehn, 2014-04-01 01:16:11]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-01 01:17:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/1

[commit-bot: I haz the power, 2014-04-01 01:36:47]

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

[commit-bot: I haz the power, 2014-04-01 01:36:47]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[esprehn, 2014-04-01 02:23:21]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-01 02:23:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/1

[commit-bot: I haz the power, 2014-04-01 02:33:46]

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

[commit-bot: I haz the power, 2014-04-01 02:33:46]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[esprehn, 2014-04-01 06:42:24]

Turns out there was a silly logic bug where I was marking ancestors of the
insertionPoint but not the insertionPoint itself. I think it's best to just
leave the code in ::childrenChanged and make the bit check properly. Putting it
in ::insertedInto is harder and also means we take that branch at every level of
the tree as we go down the appendChild which isn't needed.

[esprehn, 2014-04-01 06:44:37]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-01 06:44:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/30004

[eseidel, 2014-04-01 06:59:14]

can we test that bug?

[esprehn, 2014-04-01 07:00:17]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2014-04-01 07:00:38]

On 2014/04/01 06:59:14, eseidel wrote:
> can we test that bug?

Sure, I'll write a test tomorrow. It was caught by the :focus pseudo class unit
tests too.

[esprehn, 2014-04-02 01:08:52]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-02 01:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/90001

[commit-bot: I haz the power, 2014-04-02 01:19:34]

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

[commit-bot: I haz the power, 2014-04-02 01:19:34]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[esprehn, 2014-04-02 03:44:52]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-02 03:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/90001

[esprehn, 2014-04-02 03:45:25]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2014-04-02 20:26:26]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-02 20:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/110001

[commit-bot: I haz the power, 2014-04-02 21:06:09]

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

[commit-bot: I haz the power, 2014-04-02 21:06:09]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[esprehn, 2014-04-02 21:23:26]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-02 21:23:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/219993002/110001

[commit-bot: I haz the power, 2014-04-02 21:54:32]

Change committed as 170705

[ojan, 2014-04-03 16:55:04]

https://codereview.chromium.org/219993002/diff/110001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/219993002/diff/110001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:747: if (document().hasPendingStyleRecalc())
I think this needs a comment explaining why it's here even though it's redundant
with checks in scheduleRenderTreeUpdate. Otherwise someone will look at this
code a year from now and see that this check is redundant and remove it.