1358503 - memoize some of the anonid elements in the oneoffs binding

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Marco Bonardo [:mak]]

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 1

[Marco Bonardo [:mak]]

Attachment: Bug 1358503 - memoize anon elements in the oneoffs binding.

Review commit: https://reviewboard.mozilla.org/r/132424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132424/

Comment 2

[Florian Quèze [:florian]]

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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Florian Quèze [:florian]]

Yes, this is better. I was just saying I'm not convinced the difference will be significant enough to have a user impact.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8860413 [details]
Bug 1358503 - memoize anon elements in the oneoffs binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132424/diff/1-2/

Comment 6

[Florian Quèze [:florian]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58e7ff564f30
memoize anon elements in the oneoffs binding. r=florian

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/58e7ff564f30

Comment 9

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1358503 - Remove DataSourceSurface assertion when doing snapshot in WebGL;

Review commit: https://reviewboard.mozilla.org/r/134266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134266/

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8862349 [details]
Bug 1358503 - Remove DataSourceSurface assertion when doing snapshot in WebGL;

Sorry. This is a wrong upload