1353187 - guard access to the frame property table with a frame state bit

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ehsan pointed out that reflow profiles show us spending a lot of time accessing the frame property table.

We should guard access to the frame property table with a frame state bit to optimize this away for frames that have no properties.  (I should also run some numbers for how frequent such frames are!)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=737a3134579c5ea03088efa372d113d3c6821bec&group_state=expanded
Nothing significant showed up in perfherder, although I don't think that means much:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=03d602fd723a&newProject=try&newRevision=737a3134579c5ea03088efa372d113d3c6821bec&framework=1&filter=linux%20opt%20e10s&showOnlyImportant=0

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: measurement patch (on top of other patches here)

I used this patch to get some quick measurements.

With this patch, I started Firefox ("./mach run") which loaded the start page, went to maps.google.com, and then went to www.mozilla.org and closed Firefox.  The printfs from this patch (run through "| sort | uniq -c") added up to:

  81945 proptable get bit=0
 236706 proptable get bit=1
  44679 proptable remove bit=0
  46948 proptable remove bit=1

I think that says this is worth doing, although I was hoping it would be a little more effective than that.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Give frame properties the const-ness semantics of member variables

This makes it so that, given a |const nsIFrame*|, a caller can retrieve
properties but not set or remove them, but with an |nsIFrame*| all
operations are allowed.  I believe this is sensible since properties act
as extended member variables for things that are needed rarely, and
these are the const-ness semantics of member variables.

This also avoids the need for const_cast<nsIFrame*> to cast away const
in the following patch, which guards property access with a frame state
bit.

MozReview-Commit-ID: IJ9JnGzdH51

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Guard access to the frame property table with a frame state bit

This protects all accesses to the frame property table with a bit stored
on the frame.  This means we avoid hashtable operations when asking
about frame properties on frames that have no properties.

The changes to RestyleManager, and the new HasSkippingBitCheck API, are
needed because RestyleManager depended on being able to ask for
properties on a deleted frame (knowing that the property in question
could not have been set on any new frames since the deleted frame was
destroyed), in order to use the destruction of the properties that
happens at frame destruction as a mechanism for learning that the frame
was destroyed.  The changes there preserve the use of that mechanism,
although it becomes a bit uglier.  The ugliness is well-deserved.

MozReview-Commit-ID: BScmDUlWq65

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8854196 [details] [diff] [review]
Give frame properties the const-ness semantics of member variables

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

This is nice! r=me with typo fixed:

::: layout/base/FramePropertyTable.h
@@ +396,5 @@
> + * properties of a frame.
> + *
> + * However, since frame properties are like member variables, we have
> + * different versions for whether the frame is |const|, sharing a common
> + * a base class.

typo: drop the "a" at the start of the final line here.  (It's extra, after "a common..." on the previous line)

@@ +406,5 @@
>  public:
>    template<typename T> using Descriptor = FramePropertyTable::Descriptor<T>;
>    template<typename T> using PropertyType = FramePropertyTable::PropertyType<T>;
>  
> +  FramePropertiesBase(FramePropertyTable* aTable, CVnsIFrame* aFrame)

Perhaps the constructor should be protected, since you don't want to be able to create instances of this superclass directly, right?  (Only its subclasses should be able to invoke the constructor.)

(Probably nobody's ever actually going to try to do that, but it might be helpful from a reinforcing-the-documentation perspective. *shrug*.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8854197 [details] [diff] [review]
Guard access to the frame property table with a frame state bit

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

I need to stare at the RestyleManager.cpp changes a bit more (I want to read + understand that better before I r+ this), but assuming that all checks out, this seems good.

Some minor review notes:

::: layout/base/FramePropertyTable.cpp
@@ +87,5 @@
>      mLastFrame = aFrame;
>      mLastEntry = entry;
>    }
>  
> +  MOZ_ASSERT(entry || aSkipBitCheck, "frame has NS_FRAME_HAS_PROPERTIES set");

Right now, it takes me a bit of reverse engineering to figure out (a) how the message follows from the asserted conditions, and (b) what the point of the assertion is, since "frame has [some state bit] set" doesn't seem especially abort-worthy.

Could you wrap the message to its own line, and broaden it slightly to something like:
  "frame has NS_FRAME_HAS_PROPERTIES set, but lacks entry"

(That's a more clearly assert-worthy thing, and it's easier to figure out how it maps to the conditions & the state-of-the-world at this point in the code.)

@@ +136,5 @@
>      mLastFrame = aFrame;
>      mLastEntry = mEntries.GetEntry(aFrame);
>    }
>    Entry* entry = mLastEntry;
> +  MOZ_ASSERT(entry, "frame has NS_FRAME_HAS_PROPERTIES set");

My assert-message nitpick applies here, too. (The condition is simpler here, but the message is still unclear about why it's something worth asserting over.)

Perhaps broaden slightly to the following same language suggested above, which will still fit on one line here & which IMO encapsulates the assert-worthy condition in a more human-readable way?

@@ +230,2 @@
>    Entry* entry = mEntries.GetEntry(aFrame);
> +  MOZ_ASSERT(entry, "frame has NS_FRAME_HAS_PROPERTIES set");

Here too.

::: layout/generic/nsFrameStateBits.h
@@ +269,5 @@
>  // Frame owns anonymous boxes whose style contexts it will need to update during
>  // a stylo tree traversal.
>  FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)
>  
> +// Frame has properties in the properties hash.

Maybe slightly clearer:
  s/the properties hash/the Properties() hash table/

(For new people reading the code, there are way too many things called "properties" - JS/DOM properties, CSS properties, and then this thing. Good to be extra-clear which flavor we're referring to here.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8854197 [details] [diff] [review]
Guard access to the frame property table with a frame state bit

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

(OK, the RestyleManager.cpp changes / HasSkippingBitCheck usage make sense to me now. r=me modulo the textual suggestions in comment 6.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a86c4218ca5f2c1f5da6a0d0f415eef4f55331b6
Bug 1353187 - Give frame properties the const-ness semantics of member variables.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/74eb0b08e42bd5c0ddd9f1b140cb98880f5377e8
Bug 1353187 - Guard access to the frame property table with a frame state bit.  r=dholbert

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a56372e9dc96187eedecebf29a7ddf8d1e180e0c
Backed out changeset 74eb0b08e42b (bug 1353187 patch 2) for test failures (assertions firing).

Comment 10

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2ba7fac730edb476a51a732011872e95d34d82c8&filter-searchStr=OS+X+10.10+debug+XPCShell+(X) for failing xpcshell tests like https://treeherder.mozilla.org/logviewer.html#?job_id=88513551&repo=mozilla-inbound&lineNumber=7989 even after backout of the other patches.

to keep the tree clear backing out this changes

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/837fb46a9d4a
Backed out changeset a86c4218ca5f

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: interdiff to fix test failure

This fixes the test failure in layout/style/test/test_initial_computation.html; the Delete call also needs to be made skipping the bit check, because we can destroy a frame *after* restyling it.  (This could happen, for example, if we restyle one of its siblings, requiring a frame reconstruct, and that reconstruct needs to propagate up to the parent.  I don't think that was the case in test_initial_computation, but it is definitely a case that can happen, which I should have realized.)

I'll merge these changes into the primary patch.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8854601 [details] [diff] [review]
interdiff to fix test failure

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

r=me

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: second interdiff to fix test failure

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f3c56e5af5be0e01c02a67496ae5da91f7a8ac&group_state=expanded looks pretty green (though not *quite* done yet).

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8854642 [details] [diff] [review]
second interdiff to fix test failure

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

r=me

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3075684667085325129a6ae7fb0375e476b9036
Bug 1353187 - Give frame properties the const-ness semantics of member variables.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/63a14b2f3c7432bc2898a5f814a6ad84539cd8ad
Bug 1353187 - Guard access to the frame property table with a frame state bit.  r=dholbert

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c30756846670
https://hg.mozilla.org/mozilla-central/rev/63a14b2f3c74