Element::Register/UpdateIntersectionObservation does unnecessary hashtable lookups

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
13 days ago
10 days ago

People

(Reporter: mats, Assigned: mats)

Tracking

({perf})

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

13 days ago
Created attachment 8876492 [details] [diff] [review]
Replace calls to Contains+Put with LookupForAdd and Contains+Get+Put with LookupRemoveIf to avoid unnecessary hashtable lookups

I'm intentionally removing the local variable 'observers' since its
existence implies doing multiple hashtable operations, which is
unnecessary in general.
Attachment #8876492 - Flags: review?(nfroyd)
Comment on attachment 8876492 [details] [diff] [review]
Replace calls to Contains+Put with LookupForAdd and Contains+Get+Put with LookupRemoveIf to avoid unnecessary hashtable lookups

Review of attachment 8876492 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +4100,5 @@
> +  RegisteredIntersectionObservers()->LookupRemoveIf(aObserver,
> +    [&updated, aThreshold] (int32_t& aValue) {
> +      updated = aValue != aThreshold;
> +      aValue = aThreshold;
> +      return false; // don't remove the entry

Sort of weird that we're using LookupRemoveIf here when all we want to do is an in-place update.  Impedance mismatch of a sort.
Attachment #8876492 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

11 days ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> > +  RegisteredIntersectionObservers()->LookupRemoveIf(aObserver,
> > +    [&updated, aThreshold] (int32_t& aValue) {
> > +      updated = aValue != aThreshold;
> > +      aValue = aThreshold;
> > +      return false; // don't remove the entry
> 
> Sort of weird that we're using LookupRemoveIf here when all we want to do is
> an in-place update.  Impedance mismatch of a sort.

Yes, I agree.  I'm looking into this in bug 1372317, so the above would become
something like:

if (auto entry = Lookup(aObserver)) {
  // yay, the entry exists
  // use/modify entry.Data() here
}

I think that makes it clearer, and it also avoids trucking values
through the lambda capture clause which is a bit tedious.

Comment 4

11 days ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86463591cc9
Replace calls to Contains+Put with LookupForAdd and Contains+Get+Put with LookupRemoveIf to avoid unnecessary hashtable lookups.  r=froydnj

Comment 5

10 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e86463591cc9
Status: NEW → RESOLVED
Last Resolved: 10 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.