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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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.
(Assignee)

Comment 1

7 months ago
Created 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

ISTM this may be the simplest way to optimize these cases, keeping the code very straightforward.
Attachment #8872088 - Flags: review?(mats)
(Assignee)

Updated

7 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

Comment 2

7 months ago
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.
Attachment #8872088 - Flags: review?(mats) → review+
(Assignee)

Comment 3

7 months ago
(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.
(Assignee)

Comment 4

7 months ago
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
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
Flags: needinfo?(jfkthame)
(Assignee)

Comment 6

7 months ago
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
(Assignee)

Updated

7 months ago
Flags: needinfo?(jfkthame)
https://hg.mozilla.org/mozilla-central/rev/ac905e4037e1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.