Closed
Bug 1358503
Opened 8 years ago
Closed 8 years ago
memoize some of the anonid elements in the oneoffs binding
Categories
(Firefox :: Search, enhancement, P1)
Firefox
Search
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
Details
(Whiteboard: [photon-performance][fxsearch])
Attachments
(1 file, 1 obsolete file)
Just a minor thing, we search for these elements often, but they don't really move.
Another thing we could look into is probably getSelectableButtons(), it seems to be invoked often (on every selection basically) and every time it rebuilds a list of buttons... But that's not for this bug.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8860413 [details]
Bug 1358503 - memoize anon elements in the oneoffs binding.
https://reviewboard.mozilla.org/r/132424/#review135310
Do we have evidence that the getAnonymousElementByAttribute calls causing a perf issue here?
If I remember correctly, xbl <field>s are initialized lazily, is this correct?
I'm not really convinced this helps with performance, but I'm willing to r+ the next version because it improves readability of the code.
::: browser/components/search/content/search.xml:1563
(Diff revision 1)
>
> - let list = document.getAnonymousElementByAttribute(this, "anonid",
> - "search-panel-one-offs");
> let settingsButton =
> document.getAnonymousElementByAttribute(this, "anonid",
> "search-settings-compact");
Were you intenting to replace this one too?
::: browser/components/search/content/search.xml:1599
(Diff revision 1)
> let rowCount = Math.ceil(oneOffCount / enginesPerRow);
> let height = rowCount * 33; // 32px per row, 1px border.
> - list.setAttribute("height", height + "px");
> + this.buttons.setAttribute("height", height + "px");
>
> // Ensure we can refer to the settings buttons by ID:
> let settingsEl = document.getAnonymousElementByAttribute(this, "anonid", "search-settings");
and here
::: browser/components/search/content/search.xml:1601
(Diff revision 1)
> - list.setAttribute("height", height + "px");
> + this.buttons.setAttribute("height", height + "px");
>
> // Ensure we can refer to the settings buttons by ID:
> let settingsEl = document.getAnonymousElementByAttribute(this, "anonid", "search-settings");
> settingsEl.id = this.telemetryOrigin + "-anon-search-settings";
> let compactSettingsEl = document.getAnonymousElementByAttribute(this, "anonid", "search-settings-compact");
and here too.
Attachment #8860413 -
Flags: review?(florian) → review-
Assignee | ||
Comment 3•8 years ago
|
||
looks like my search-foo missed some entries (I know why, I started doing that and then noticed the coalesced property for settingsButton and ended up forgetting the search & replace).
https://enndeakin.wordpress.com/2011/10/04/xbl-performance-tips/
"Fields and handlers (<field> and <handler> elements) however are not compiled when the element is added. Instead, they are compiled on demand when they are used. This means that both fields and handlers do not directly affect the time to open a window, but can instead, in the case of handlers, affect the time to respond to an event."
I don't have a clear evidence, but we do multiple calls for oneoffs and every time we go through the getAnonymousElement path. that sounds like a waste.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: -- → P3
Comment 4•8 years ago
|
||
Yes, this is better. I was just saying I'm not convinced the difference will be significant enough to have a user impact.
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8860413 [details]
Bug 1358503 - memoize anon elements in the oneoffs binding.
https://reviewboard.mozilla.org/r/132424/#review135316
r=me, please run at least the browser/components/search/test tests before pushing this.
Attachment #8860413 -
Flags: review?(florian) → review+
Updated•8 years ago
|
Flags: qe-verify?
Priority: P3 → P1
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/58e7ff564f30
memoize anon elements in the oneoffs binding. r=florian
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•8 years ago
|
Iteration: --- → 55.4 - May 1
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
Comment on attachment 8862349 [details]
Bug 1358503 - Remove DataSourceSurface assertion when doing snapshot in WebGL;
Sorry. This is a wrong upload
Attachment #8862349 -
Flags: review?(jgilbert)
Updated•8 years ago
|
Attachment #8862349 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•