1216431 - revisit approach to fixing bug 1209603

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

In hindsight, I think I made the wrong choice when fixing bug 1209603.  CalcStyleDifference's optimizations related to PeekStyleData aren't really that valuable, since they really only help for elements that haven't yet been displayed.  So I think we should replace some of the patches there with the alternative I described in bug 1209603 comment 62.

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a87c755197
https://treeherder.mozilla.org/perf.html#/comparechooser?originalProject=mozilla-central&originalRevision=f7b746b4e913&newProject=try&newRevision=f5a87c755197

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: stack showing reason for test failures

With this approach we end up with test failures because we're computing a new nsStyleVisibility inside of nsStylePosition::CalcDifference.

Presumably with the current approach we end up not calling nsStylePosition::CalcDifference because having PeekStyleData be exact means we don't call it.

Interestingly, nsRuleNode::ComputePositionData gets the writing mode and does things as a function of it, but *doesn't* set conditions properly depending on it.  So there are some other latent bugs near there.

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #2)
> Interestingly, nsRuleNode::ComputePositionData gets the writing mode and
> does things as a function of it, but *doesn't* set conditions properly
> depending on it.  So there are some other latent bugs near there.

I filed bug 1216747 on this.

Comment 4

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df9b2b732b5e
https://treeherder.mozilla.org/perf.html#/comparechooser?originalProject=mozilla-central&originalRevision=f7b746b4e913&newProject=try&newRevision=df9b2b732b5e

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd5be722630
https://treeherder.mozilla.org/perf.html#/comparechooser?originalProject=mozilla-central&originalRevision=f7b746b4e913&newProject=try&newRevision=4cd5be722630

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

https://hg.mozilla.org/try/pushloghtml?changeset=d4ba3dc65504
https://treeherder.mozilla.org/perf.html#/comparechooser?originalProject=mozilla-central&originalRevision=f7b746b4e913&newProject=try&newRevision=d4ba3dc65504

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 1 - Give WritingMode an nsStyleVisibility* constructor

This is needed for patch 2.

Comment 8

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 2 - Pass nsStyleVisibility to nsStylePosition::CalcDifference to avoid computing new nsStyleVisibility during nsStyleContext::CalcStyleDifference

This is needed to avoid hitting the assertion:
Assertion failure: !!(structsFound & (1 << uint64_t(eStyleStruct_Visibility))) == !!PeekStyleVisibility() (PeekStyleData results must not change in the middle of difference calculation.), at ./nsStyleStructList.h:62
once exact PeekStyleData is backed out in the later patches.

Without this patch, we can compute a new nsStyleVisibility struct inside
of nsStylePosition::CalcDifference.  This patch ensures we use
PeekStyleVisibility() instead of StyleVisibility().

Comment 9

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 3 - Cache structs that are stored with conditions on the rule node all the time, rather than only when freshly computed

This is another case similar to the problem fixed in bug 1209603 patch 9.

Comment 10

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 4 - Don't trigger computation of new structs via testing of conditions for conditionally-stored structs on the rule node

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 5 - Back out half of bug 1209603 patch 10

This leaves the half related to inherited structs, since we can still
bail early for them even without mBits.

Comment 12

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 6 - Back out bug 1209603 patch 8

This requires a little bit of gymnastics since it has to add the inverse
of tests, since the is-a-reset-struct tests originally added in patch 8
were made unconditional in patch 9, and with this backout we now want to
execute the code only for inherited structs.

This also doesn't back out the cleanup to use NS_STYLE_INHERIT_BIT() for
constants rather than nsCachedStyleData::GetBitForSID.

Comment 13

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 7 - Back out bug 1209603 patch 6

This backs out all of the patch except that it retains some of the
comment changes for nsStyleContext::mBits.

Comment 14

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Oh, and the better performance comparison URL is:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=f7b746b4e913&newProject=try&newRevision=d4ba3dc65504

which is probably best viewed along with the one from bug 1209603:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=833c3c37daa6&newProject=try&newRevision=98d531d40668

I wish I could merge them...

Comment 15

[Cameron McCormack (:heycam) (away 21 Dec – 6 Jan)]

Comment on attachment 8677182 [details] [diff] [review]
patch 4 - Don't trigger computation of new structs via testing of conditions for conditionally-stored structs on the rule node

Review of attachment 8677182 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe aComputeData on GetStyleData should be called something else, since the existing aComputeData (template) parameter means "if we don't have the struct cached, compute it", whereas here it's being used to mean "if we need to compute a dependent struct, allow that".

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f31d5853b0a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e9257c073d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2856d00e310a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5381b658ae64
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9f6b8b11a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/063bf9fb0cd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/713de8f859af

Comment 17

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

(In reply to Cameron McCormack (:heycam) (away Oct 28) from comment #15)
> Maybe aComputeData on GetStyleData should be called something else, since
> the existing aComputeData (template) parameter means "if we don't have the
> struct cached, compute it", whereas here it's being used to mean "if we need
> to compute a dependent struct, allow that".

I renamed to aCanComputeData; I hope you think that's an improvement.

Comment 18

[Cameron McCormack (:heycam) (away 21 Dec – 6 Jan)]

Yes, that's an improvement, thanks.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f31d5853b0a0
https://hg.mozilla.org/mozilla-central/rev/41e9257c073d
https://hg.mozilla.org/mozilla-central/rev/2856d00e310a
https://hg.mozilla.org/mozilla-central/rev/5381b658ae64

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e18c0d373b1f

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e18c0d373b1f

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e18c0d373b1f