Closed Bug 1513393 Opened 9 months ago Closed 8 months ago

Simplify/optimize some frame property usage

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: perf)

Attachments

(5 files)

Filing this bug on some misc frame property cleanup in layout (largely in painting-related code, so I'll tag mattwoodrow to review).

The first patch here removes a hand-coded DTOR (just removing unnecessary code).

And the rest of the patches are all adjusting instances of this pattern:
>  Foo* foo = GetProperty(PropName());
>  if (!foo) {
>    foo = new Foo();
>    SetProperty(PropName(), foo);
>  }
...to use AddProperty instead of SetProperty.

AddProperty is a bit faster in this scenario, because it assumes [correctly] that the property does not already exist so it can be appended to the end of the list. Whereas SetProperty does a [redundant in this case] walk through the list to see if there's an existing value that it should stomp on.

To have maximal assurance about AddProperty's "property does not already exist" precondition, my patches will also be making use of the optional "found" boolean outparam that GetProperty() exposes, so that we can distinguish between "property-not-found" vs. "property-found-and-the-stored-value-is-null" (unexpected here but theoretically possible).
Keywords: perf
(Note: this bug has a soft dependency on bug 1513387, in that some of the patches touch the same contextual code, and I've ordered things with bug 1513387 modifying that code first and then this bug modifying it second.

So if you happen to want to apply these patches locally e.g. as part of review, you'll likely need bug 1513387's patch first.)
Depends on: 1513387
Previously we were providing a hand-coded destructor for this type, but this
was unnecessary, because the default one (invoked by 'delete') will do the same
thing.
Previously we were using SetProperty, which does a redundant walk through the
property list to see whether the property already exists. This was unnecessary
because we only invoke it in the scenario when the property wasn't set.

So, we can use AddProperty to simply append and skip the redundant walk. And to
be extra-sure that the frame property is not present, this patch makes us use
the stricter 'found' outparam that our GetProperty() API provides (to
differentiate between no-result vs. a null value being intentionally stored
[unexpected but theoretically possible, & now checked for via MOZ_ASSERT].)

Depends on D14221
As in the previous commit, this avoids a redundant walk through the list of
frame properties, when we already know the property is not there.

Depends on D14222
As in the previous commit, this avoids a redundant walk through the list of
frame properties, when we already know the property is not there.

Depends on D14223
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1a260c80b39
part 1: Use NS_DECLARE_FRAME_PROPERTY_DELETABLE (not explicit dtor) to declare frame property PaintedPresShellsProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/56dfe60d175a
part 2: In "retrieve-or-initialize" dance for PaintedPresShellsProperty, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/734fa7761481
part 3: In "retrieve-or-initialize" dance for DisplayListBuildingDisplayPortRect property, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/b87cc4036ec6
part 4: In "retrieve-or-initialize" dances in SVGObserverUtils.cpp, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d235fd42709b
part 5: Add a utility function to set or update nsRect-pointer-valued frame properties. r=mattwoodrow
You need to log in before you can comment on or make changes to this bug.