1368249 - Provide more efficient FrameProperties API to replace use of GetProperty followed by SetProperty

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Kew [:jfkthame]]

From bug 1365982 comment 14:
----------------------------
>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.
----------------------------

A LookupOrAdd() type of API would be one option; or perhaps simpler would be to create an AddProperty() that unconditionally appends the new property, without searching for an existing one. This would be for use only when the caller knows for sure that there is no existing entry (typically because it just called GetProperty).

To catch potential misuse, debug builds could have an assertion in AddProperty() to check that there isn't an existing entry.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Create nsIFrame::AddProperty API for use when the property is known to not already exist, and use to optimize some call sites

ISTM this may be the simplest way to optimize these cases, keeping the code very straightforward.

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8872088 [details] [diff] [review]
Create nsIFrame::AddProperty API for use when the property is known to not already exist, and use to optimize some call sites

>layout/base/FrameProperties.h
>   /**
>+   * Set a property value. This requires a linear search through
>+   * the properties of the frame. Any existing value for the property
>+   * is destroyed.
>+   */
>+  template<typename T>
>+  void Add(Descriptor<T> aProperty, PropertyType<T> aValue)

The documentation doesn't look right.


>layout/generic/nsFrame.cpp
>+nsIFrame::GetNormalPosition(bool* aHasProperty) const
> {
>   // It might be faster to first check
>   // StyleDisplay()->IsRelativelyPositionedStyle().
>   nsPoint* normalPosition = GetProperty(NormalPositionProperty());
>   if (normalPosition) {
>+    if (aHasProperty) {
>+      *aHasProperty = true;
>+    }
>     return *normalPosition;
>   }
>+  if (aHasProperty) {
>+    *aHasProperty = false;
>+  }
>   return GetPosition();
> }

I think it's worth moving this method to nsIFrameInlines.h now, so that
these aHasProperty null-checks can be optimized away.

Also, I suggest we remove the "It might be faster..." comment now,
because I don't think using StyleDisplay() here will pay off.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #2)
> Comment on attachment 8872088 [details] [diff] [review]
> Create nsIFrame::AddProperty API for use when the property is known to not
> already exist, and use to optimize some call sites
> 
> >layout/base/FrameProperties.h
> >   /**
> >+   * Set a property value. This requires a linear search through
> >+   * the properties of the frame. Any existing value for the property
> >+   * is destroyed.
> >+   */
> >+  template<typename T>
> >+  void Add(Descriptor<T> aProperty, PropertyType<T> aValue)
> 
> The documentation doesn't look right.

Oops, thanks for catching! (I'm sure I remember changing that... must have messed up somewhere.) Will fix.

> >layout/generic/nsFrame.cpp
> >+nsIFrame::GetNormalPosition(bool* aHasProperty) const
> 
> I think it's worth moving this method to nsIFrameInlines.h now, so that
> these aHasProperty null-checks can be optimized away.

Makes sense. Will do.

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c0589220ecb10b820a5c165e33bfb0f98720a6
Bug 1368249 - Create nsIFrame::AddProperty API for use when the property is known to not already exist, and use to optimize some call sites. r=mats

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for unused function GetNormalPosition at nsIFrame.h:1073:

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

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=59c0589220ecb10b820a5c165e33bfb0f98720a6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=102664807&repo=mozilla-inbound

/home/worker/workspace/build/src/intl/icu/source/common/ucnv2022.cpp
[task 2017-05-28T12:29:26.406992Z] 12:29:26     INFO -  In file included from /home/worker/workspace/build/src/layout/base/AccessibleCaretEventHub.h:14:0,
[task 2017-05-28T12:29:26.407244Z] 12:29:26     INFO -                   from /home/worker/workspace/build/src/layout/base/gtest/TestAccessibleCaretEventHub.cpp:13,
[task 2017-05-28T12:29:26.407443Z] 12:29:26     INFO -                   from /home/worker/workspace/build/src/obj-firefox/layout/base/gtest/Unified_cpp_layout_base_gtest0.cpp:2:
[task 2017-05-28T12:29:26.407639Z] 12:29:26     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:1073:18: error: inline function 'nsPoint nsIFrame::GetNormalPosition(bool*) const' used but never defined [-Werror]
[task 2017-05-28T12:29:26.407738Z] 12:29:26     INFO -     inline nsPoint GetNormalPosition(bool* aHasProperty = nullptr) const;
[task 2017-05-28T12:29:26.407904Z] 12:29:26     INFO -                    ^
[task 2017-05-28T12:29:26.408067Z] 12:29:26     INFO -  cc1plus: all warnings being treated as errors
[task 2017-05-28T12:29:26.408210Z] 12:29:26     INFO -  /home/worker/workspace/build/src/config/rules.mk:1063: recipe for target 'Unified_cpp_layout_base_gtest0.o' failed
[task 2017-05-28T12:29:26.408362Z] 12:29:26     INFO -  gmake[5]: *** [Unified_cpp_layout_base_gtest0.o] Error 1

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac905e4037e15b594d90703d87874ac0a91f557f
Bug 1368249 - Create nsIFrame::AddProperty API for use when the property is known to not already exist, and use to optimize some call sites. r=mats

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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