1372007 - Element::Register/UpdateIntersectionObservation does unnecessary hashtable lookups

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Mats Palmgren (inactive)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e86463591cc9