1365982 - Attach FrameProperties to each frame instead of using a shared hashtable

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Kew [:jfkthame]]

Although we've recently done some work to reduce the amount of FramePropertyTable lookup we do, it still shows up as a significant element in many profiles.

For example, in https://perfht.ml/2rv39uP, which is loading the single-page HTML spec, there is 355ms spent in FramePropertyTable methods; around half of this is spent in PLDHashTable methods, i.e. looking up the frame pointer in the hashtable to find its list of properties.

What I'd like to consider here is dispensing with the shared hashtable and instead attaching the frame property list directly to nsIFrame. This will add one pointer to the size of all frame classes, but will completely eliminate the hashtable cost here.

One thing that may be tricky is that last time I looked, we had some code that depends on being able to safely do a property lookup for a possibly-deleted nsIFrame pointer. Any such usage will need to be reworked if the properties are directly attached to the frame.

Comment 1

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

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> One thing that may be tricky is that last time I looked, we had some code
> that depends on being able to safely do a property lookup for a
> possibly-deleted nsIFrame pointer. Any such usage will need to be reworked
> if the properties are directly attached to the frame.

Yeah, that code is the code in RestyleManager that I had to patch in https://hg.mozilla.org/mozilla-central/rev/63a14b2f3c74 with the HasSkippingBitCheck API.  Based on that patch, I don't think there's any other code doing that.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Here's my current WIP for this, which seems to be working pretty well; still needs some more testing, though.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Given that hashtable lookups associated with the FramePropertyTable often show up in profiles, sometimes as much as 1-2% of overall time spent in reflow, it seems like it may be worth eliminating this overhead. I think this now passes all tests... pushed a try job to check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8902f0ccdabdd5283b57aa27cf1cf35858915a1

Comment 4

[Jonathan Kew [:jfkthame]]

BTW, with this patch, the NS_FRAME_HAS_PROPERTIES bit becomes obsolete, so we can free that one up; I'll post a followup patch to do that.

I also want to check if there are places we should add const-ness so that we don't risk creating a new (empty) properties array when we're only going to Get a property, and not actually Set anything.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8870596 [details] [diff] [review]
Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Cancelling r? for now, as I think this can be made significantly better.... improved patch coming.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Wrap FrameProperties access with checks to avoid creating the properties object until there is something to put in it

Comment 8

[Jonathan Kew [:jfkthame]]

Apologies for the size of the patches! The bulk of each of them is semi-mechanical updates to FrameProperties call sites; the interesting stuff is mainly the actual FrameProperties files, and changes to nsIFrame.h.

Part 1 works correctly by itself, and eliminates the hashtable, but has the problem that we end up creating a FrameProperties object for just about every frame, even if we don't ever set any properties on it; it still seems to give a modest perf win, but not as good as it should be because of that flaw. Part 2 fixes that, and thus shaves some more cycles (and allocations). Overall, I'm seeing typical frame-property-related timings during the HTML5 spec load cut from something over 200ms to well under 100ms.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8871038 [details] [diff] [review]
part 1 - Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

> const nsIFrame* const mFrame;

I really don't think we should store a frame pointer *permanently*
in the object that stores the properties.

The way that the current code works is that FrameProperties stores
it but this is a *transient* object that only exists while getting/
adding/removing properties.  Why can't we keep it that way?

Comment 10

[Jonathan Kew [:jfkthame]]

We can do that, yes. The frame pointer is needed when we delete (or replace) a property, for potentially passing to the property destructor, so if we remove it from FrameProperties then we'll need to pass it whenever we call Set or Delete, but that's perfectly feasible, just a bit more churn at call sites.

What I'd prefer to do, I think, is to handle that in an additional "part 3" patch, so that there's less change happening in any one of these big patches. (That's also why I didn't fold together the two patches already here.) Is it OK with you to implement that way?

Comment 11

[Mats Palmgren (inactive)]

No problem, but please post a rollup patch to make it easier to review the final result.  Thanks!

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - Remove the frame pointer from the FrameProperties object, and just pass it to methods that need it

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: rollup of parts 1-3 - Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8871435 [details] [diff] [review]
rollup of parts 1-3 - Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

>layout/base/FrameProperties.cpp
>+FrameProperties::GetInternal(UntypedDescriptor aProperty, bool* aFoundResult) const
>+  if (index == nsTArray<PropertyValue>::NoIndex) {
>+    if (aFoundResult) {
>+      *aFoundResult = false;
>+    }

It might be worth moving some of these methods to a FramePropertiesInlines.h
file to allow the compiler to optimize better.  E.g. the above if-statement
can be removed if the call was GetProperty(property) and the null-check
can be suppressed for GetProperty(property, &someBool).

>layout/base/FrameProperties.h
>+  bool Has(Descriptor<T> aProperty) const
>   {
>     bool foundResult = false;
>+    mozilla::Unused << GetInternal(aProperty, &foundResult);
>     return foundResult;

I doubt the compiler smart enough to optimize this well.  I think we should
help it with something like this instead:
> return mProperties.IndexOf(...) != NoIndex;

>+   * Stores a property descriptor/value pair.
>    */
>   struct PropertyValue {

A digression unrelated to the review: I've always thought that it's
a waste of space to store a pointer to the descriptor for every value.
We don't have that many properties, so we could put them all in an
enum which will likely fit in one byte here, and then get the descriptor
through an array indexed by the enum.

The problem with implementing that efficiently is that with an array of
PropertyValue the alignment requirement for the value is likely to eat up
the potential gain, so we'd need to have two separate arrays, one for
the descriptor keys and one for the values.

This seems worth looking into at some point so I thought I'd mention it...

>     size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
> ...
>+      // Sorry!
>+      return 0;

Can we remove this method?  Or do you want to keep it around to address
the above at some point?  (you should probably add a XXX if so)

>+  AutoTArray<PropertyValue,1> mProperties;

This is fine for now, but it does seem a bit wasteful to use an AutoTArray
to store a single property, because of the array header overhead.  We can
probably store properties more memory-efficient in general, but perhaps
it's not worth the extra code complexity...

Also, it would be nice if we could allocate these things from the shell
arena at some point.


>layout/base/GeckoRestyleManager.cpp
>@@ -1010,17 +1010,17 @@ GeckoRestyleManager::ReparentStyleContex
>         nsIFrame* sib =
>-          aFrame->Properties().Get(nsIFrame::IBSplitSibling());
>+          aFrame->GetProperty(nsIFrame::IBSplitSibling());
>         if (sib) {
>           ReparentStyleContext(sib);
>         }

Looks like it fits on one line now?

>@@ -3023,32 +3023,31 @@ ElementRestyler::ComputeStyleChangeFor(n
>     nextIBSibling =
>-      GeckoRestyleManager::GetNextBlockInInlineSibling(propTable, ibSibling);
>+      GeckoRestyleManager::GetNextBlockInInlineSibling(ibSibling);

Maybe this one too?


>layout/base/RestyleManager.cpp
>@@ -801,20 +799,19 @@ RecomputePosition(nsIFrame* aFrame)
>         nsPoint normalPosition = cont->GetNormalPosition();
>+        if (!cont->GetProperty(nsIFrame::NormalPositionProperty())) {
>+          cont->SetProperty(nsIFrame::NormalPositionProperty(),
>+                            new nsPoint(normalPosition));

It looks like we search for the property at least 2, maybe 3 times here.
We should invent something along the lines of the LookupOrAdd or
LookupForAdd.OrInsert things we have for hash tables.
File a follow-up bug on this?


>+RestyleManager::GetNextBlockInInlineSibling(nsIFrame* aFrame)
>   return static_cast<nsIFrame*>
>-    (aPropTable->Get(aFrame, nsIFrame::IBSplitSibling()));
>+    (aFrame->GetProperty(nsIFrame::IBSplitSibling()));

The static_cast isn't needed, please remove it while we're here.

>+#ifdef DEBUG
>   // cleanup references and verify the style tree.  Note that the latter needs
>   // to happen once we've processed the whole list, since until then the tree
>   // is not in fact in a consistent state.

Could you update the comment please?  I think the "cleanup references"
part is about the code that you removed.  So I think this can say:
"Verify the style tree.  This needs to happen ..."

>+  mDestroyedFrames.reset(nullptr);

I think this should probably go after the "frameConstructor->EndUpdate()"
line instead, to be more visible.

BTW, nice improvement with the mDestroyedFrames!

>layout/base/nsCSSFrameConstructor.cpp
>   return static_cast<nsContainerFrame*>
>     (aFrame->FirstContinuation()->
>-       Properties().Get(nsIFrame::IBSplitSibling()));
>+       GetProperty(nsIFrame::IBSplitSibling()));

I wonder if we could declare the IBSplitSibling property as
nsContainerFrame* instead and avoid these casts?

> AddGenConPseudoToFrame(nsIFrame* aOwnerFrame, nsIContent* aContent)
>+  nsIFrame::ContentArray* value =
>+    aOwnerFrame->GetProperty(nsIFrame::GenConProperty());
>   if (!value) {
>     value = new nsIFrame::ContentArray;
>-    props.Set(nsIFrame::GenConProperty(), value);
>+    aOwnerFrame->SetProperty(nsIFrame::GenConProperty(), value);

Stuff like this can be optimized if we had that LookupOrAdd thing...

>layout/forms/nsTextControlFrame.cpp
>-  EditorInitializer* initializer = Properties().Get(TextControlInitializer());
>+  EditorInitializer* initializer = GetProperty(TextControlInitializer());
>   if (initializer) {
>     initializer->Revoke();
>-    Properties().Delete(TextControlInitializer());
>+    DeleteProperty(TextControlInitializer());

Stuff like this can also be improved if we had something like
FrameProperties::MaybeDelete that takes a callback that's invoked
if the property exists, like so:
MaybeDeleteProperty([] (EditorInitializer*& aValue) {
  aValue->Revoke();
  return true;  // means "delete it"
});

In this particular case though, we could add a ~EditorInitializer()
that Revokes and just use DeleteProperty(TextControlInitializer()) above.

>-    EditorInitializer* initializer = Properties().Get(TextControlInitializer());
>+    EditorInitializer* initializer = GetProperty(TextControlInitializer());
>     if (initializer) {
>       initializer->Revoke();
>     }
>     initializer = new EditorInitializer(this);
>-    Properties().Set(TextControlInitializer(),initializer);
>+    SetProperty(TextControlInitializer(),initializer);

This could also avoid the first Get if the dtor Revokes.

>layout/generic/ReflowInput.cpp

Lots of the Get+Set pattern that could be optimized in this file.
(I'll stop commenting every time I see this now, since it appears this is
quite common, but please file a follow-up bug to improve this in general.)


>layout/generic/nsBlockFrame.cpp
> nsBlockFrame::GetOutsideBulletList() const
>   nsFrameList* list =
>-    Properties().Get(OutsideBulletProperty());
>+    GetProperty(OutsideBulletProperty());

Looks like it fits on one line.

> nsBlockFrame::GetPushedFloats() const
> {
>   if (!HasPushedFloats()) {
>     return nullptr;
>   }
>   nsFrameList* result =
>-    Properties().Get(PushedFloatProperty());
>+    GetProperty(PushedFloatProperty());

Ditto.


>layout/generic/nsFlexContainerFrame.cpp
>+  if (const auto* cachedResult =
>+    aItem.Frame()->GetProperty(CachedFlexMeasuringReflow())) {

I'd prefer the indentation like this:

>+  if (const auto* cachedResult =
>+        aItem.Frame()->GetProperty(CachedFlexMeasuringReflow())) {


>layout/generic/nsFrame.cpp
> nsFrame::~nsFrame()
>+  DeleteAllProperties();

Hmm, it feels a bit late to do this in the dtor.  Can we do this at
the end of nsFrame::DestroyFrom instead (where we're currently calling
shell->NotifyDestroyingFrame)?  That would be closer to what we're
currently doing.
(we can assert that we have no properties in ~nsFrame)

BTW, please update the comments in nsFrame::DestroyFrom that talks
about "...NotifyDestroyingFrame(), which will clear frame properties..."

> nsIFrame::InvalidateFrameSubtree(uint32_t aDisplayItemKey)
>-  nsRect* rect = Properties().Get(InvalidationRect());
>+  nsRect* rect = GetProperty(InvalidationRect());

Hmm, shouldn't we check HasAnyStateBits(NS_FRAME_HAS_INVALID_RECT)
before doing this Get? (and assert a non-null result in that case)
Please file a follow-up bug if you agree.

>@@ -7522,17 +7516,17 @@ nsFrame::GetPointFromOffset(int32_t inOf
>       FrameBidiData bidiData =
>-        Properties().Get(BidiDataProperty(), &hasBidiData);
>+        GetProperty(BidiDataProperty(), &hasBidiData);

Looks like it fits on one line now.


>layout/generic/nsIFrame.h
 >   mozilla::FrameBidiData GetBidiData() const
>   {
>     bool exists;
>     mozilla::FrameBidiData bidiData =
>-      Properties().Get(BidiDataProperty(), &exists);
>+      GetProperty(BidiDataProperty(), &exists);

Looks like it fits on one line now.

>+  template<typename T>
>+  FrameProperties::PropertyType<T>
>+  GetProperty(FrameProperties::Descriptor<T> aProperty,
>+              bool* aFoundResult = nullptr) const
>+  {
>+    if (mProperties) {
>+      return mProperties->Get(aProperty, aFoundResult);
>+    } else {

else after return

>+  bool HasProperty(FrameProperties::Descriptor<T> aProperty) const
>+  {
>+    if (mProperties) {
>+      return mProperties->Has(aProperty);
>+    } else {

ditto

>+  RemoveProperty(FrameProperties::Descriptor<T> aProperty,
>+                 bool* aFoundResult = nullptr)
>+  {
>+    if (mProperties) {
>+      return mProperties->Remove(aProperty, aFoundResult);
>+    } else {

ditto

>+  mozilla::UniquePtr<FrameProperties> mProperties;

This is the wrong place to add this member because it introduces a 2-byte
alignment spill.  After |mState| is probably the right place to add it.


>layout/painting/ActiveLayerTracker.cpp
> static LayerActivity*
> GetLayerActivity(nsIFrame* aFrame)
> {
>   if (!aFrame->HasAnyStateBits(NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)) {
>     return nullptr;
>   }
>+  return aFrame->GetProperty(LayerActivityProperty());
> }
> 
> static LayerActivity*
> GetLayerActivityForUpdate(nsIFrame* aFrame)
> {
>+  LayerActivity* layerActivity = aFrame->GetProperty(LayerActivityProperty());
>   if (layerActivity) {

We should probably check NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY before
calling GetProperty() in GetLayerActivityForUpdate?
Maybe they meant to call GetLayerActivity(aFrame) ?
Please file a follow-up bug if you agree.


>layout/svg/SVGTextFrame.cpp
>-  frame->Properties().Set(TextNodeCorrespondenceProperty(),
>+  frame->SetProperty(TextNodeCorrespondenceProperty(),
>                           new TextNodeCorrespondence(undisplayed));

indentation


So, overall this looks great.  I'd really like to delete the properties
in nsFrame::DestroyFrom instead of the dtor though.  So r+ on the condition
that you preserve that.  We can always move it to the dtor later if we feel
like it should be safe, but I'm not comfortable doing that change here.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

Updated patch ready for landing, with comments addressed; carrying forward r=mats. I will also file followups for suggested improvements, but plan to go ahead and land this while the tree's fairly quiet, and before it bitrots.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8872030 [details] [diff] [review]
Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext

(r+ carried forward from attachment 8871435 [details] [diff] [review])

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b37e4d256cd6c88b48b0223113375f889a748982
Bug 1365982 - Attach frame properties to each frame instead of looking them up in a hashtable on the prescontext. r=mats

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #14)
> 
> >+  AutoTArray<PropertyValue,1> mProperties;
> 
> This is fine for now, but it does seem a bit wasteful to use an AutoTArray
> to store a single property, because of the array header overhead.  We can
> probably store properties more memory-efficient in general, but perhaps
> it's not worth the extra code complexity...

I wondered about that, too, but felt that code simplicity was probably more important at this point.

Actually, I've also wondered whether we should make the auto-buffer here slightly larger, so that a separate allocation for the array contents becomes rarer (at the cost, obviously, of more "wasted" space in the cases where fewer properties are actually present, or where there are so many that we still overflow to a separate buffer).

This seems like it might be worth further investigation in a followup bug.

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1365982 followup - Update FrameProperties::Has() document. r?jfkthame

The HasSkippingBitCheck variant has been removed in
https://hg.mozilla.org/mozilla-central/rev/b37e4d256cd6

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df8dd2a9cf43
followup - Update FrameProperties::Has() document. r=jfkthame

Comment 22

[Cosmin Sabou [:CosminS]]

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