Closed Bug 1429959 Opened 2 years ago Closed 2 years ago

Have nsBindingManager::EnumerateBoundContentBindings invoke callback for each binding only once

Categories

(Core :: XBL, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

nsBindingManager::EnumerateBoundContentBindings currently enumerates all bound elements and invokes the callback with their binding and base bindings, which means it can return the same binding multiple times if there are multiple elements with the same binding (or multiple bindings sharing the same base binding).

It is pointless, and may add extra work along the callsites, since they would need to do the same thing multiple times. Since chrome is pretty XBL-intensive at the moment, I would expect this to affect performance of stylo-chrome.

(There is a risk that some callsites may depend on the order returned, e.g. base bindings always appear after their sub-bindings. If that's true, we may need to build a inheritance tree before start invoking the callbacks. But we will see whether that's a problem.)
Assignee: nobody → xidorn+moz
Priority: -- → P2
Comment on attachment 8942045 [details]
Bug 1429959 - Make nsBindingManager::EnumerateBoundContentBindings yield each binding only once.

https://reviewboard.mozilla.org/r/212258/#review218138

::: dom/xbl/nsBindingManager.cpp:740
(Diff revision 1)
> +        break;
> +      }
>        if (!aCallback(binding)) {
>          return false;
>        }
> +      bindings.PutEntry(binding);

This should be able to avoid the double hash lookup, in rust `insert()` in a hash table returns whether it was already present. It doesn't seem like nsTHashTable contains an API like that, but maybe worth filing a bug?

::: dom/xbl/nsBindingManager.cpp:740
(Diff revision 1)
> +        break;
> +      }
>        if (!aCallback(binding)) {
>          return false;
>        }
> +      bindings.PutEntry(binding);
Actually there is an EnsureInserted which seems to meet the requirement... I didn't see that when I wrote the patch.
Comment on attachment 8942045 [details]
Bug 1429959 - Make nsBindingManager::EnumerateBoundContentBindings yield each binding only once.

Emilio reviewed the original implementation of this function, along with all the consumers. So I'm happy to delegate this one to him.
Attachment #8942045 - Flags: review?(bobbyholley) → review?(emilio)
Comment on attachment 8942045 [details]
Bug 1429959 - Make nsBindingManager::EnumerateBoundContentBindings yield each binding only once.

https://reviewboard.mozilla.org/r/212258/#review218396

r=me, thanks!
Attachment #8942045 - Flags: review?(emilio) → review+
Pushed by xquan@mozilla.com
https://hg.mozilla.org/integration/autoland/rev/a16df11dae3fba36cb958b06917f35d9d0591f78
Make nsBindingManager::EnumerateBoundContentBindings yield each binding only once. r=emilio
https://hg.mozilla.org/mozilla-central/rev/a16df11dae3f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.