1376498 - Add nsInterfaceHashtable::Remove and make other hashtable Remove methods consistent

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Mats Palmgren (inactive)]

Currently you have to do something like this:

nsCOMPtr<nsISomething> value;
if (auto entry = hashtable.Lookup(key)) {
  value = entry.Data().forget();
  entry.Remove();
  ...
}

which is a bit klunky.  This would be simpler:

nsCOMPtr<nsISomething> value;
if (hashtable.Remove(key, getter_AddRefs(value))) {
  ...
}

Comment 1

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ae2d196c663d27f636e95550794c5b24a91bc0a

Comment 2

[Mats Palmgren (inactive)]

Attachment: Unify the ns[Base|Interface|RefPtr]Hashtable::Remove() signatures for consistency

(nsClassHashtable comes in a later part)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Use plain Remove(key) instead of Lookup(key).Remove() in some places for simplicity

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - Make nsClassHashtable inherit Remove() for API consistency with ns[Base|Interface|RefPtr]Hashtable

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - Make all nsClassHashtable::RemoveAndForget() consumers use Remove() instead

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 5 - Remove nsClassHashtable::RemoveAndForget()

Comment 7

[Mats Palmgren (inactive)]

A note about the first hunk in part 4: there's a minor semantic change there -
now checking for the existence of the entry rather than non-nullness of
its value, but I checked that null is never added to this hashtable:
http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/dom/base/CustomElementRegistry.cpp#299-300
so I figured it was worth it to make the code more idiomatic.
I added a MOZ_ASSERT(value) to make that invariant explicit.

Comment 8

[Mats Palmgren (inactive)]

From an API consistency POV, this makes nsDataHashtable::GetAndRemove
an aberration:
http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/xpcom/ds/nsDataHashtable.h#47

There's very few uses of that:
http://searchfox.org/mozilla-central/search?q=symbol:_ZN15nsDataHashtable12GetAndRemoveENS_7KeyTypeE&redirect=false
so I'm leaning towards converting those to also use Remove so that
we uniformly use the same Remove API everywhere.

WDYT?

Comment 9

[Mats Palmgren (inactive)]

... alternatively, we could add GetAndRemove to the other hashtable
classes too, so that it's consistently available everywhere or not
at all.  It's slightly more ergonomic than Remove() in some cases. e.g.

  return stylesheets.GetAndRemove(raw).valueOr(nullptr);

vs.

  ServoStyleSheet* sheet;
  stylesheets.Remove(raw, &sheet);
  return sheet;

Comment 10

[Mats Palmgren (inactive)]

... when I look through the existing uses of GetAndRemove though,
the one mentioned above is the only one that's motivated.  The other
ones can easily use Remove instead.  So perhaps it's not worth it
to have two ways to remove something for this one case?

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8882815 [details] [diff] [review]
Unify the ns[Base|Interface|RefPtr]Hashtable::Remove() signatures for consistency

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

r=me, but I want to double-check the rationale on the not-found behavior for Remove().

::: xpcom/ds/nsBaseHashtable.h
@@ +159,4 @@
>     * @param aKey the key to remove from the hashtable
> +   * @param aData where to move the value (if non-null).  If an entry is not
> +   *              found, *aData will be assigned a default-constructed value
> +   *              (i.e. reset to zero or nullptr for primitive types).

This seems like a really weird contract; I would expect that *aData would not be touched if false was returned.  I guess it saves a branch in some cases?

Comment 12

[Mats Palmgren (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> This seems like a really weird contract; I would expect that *aData would
> not be touched if false was returned.

I agree, but I wanted to be consistent with the existing contract for
ns[RefPtr|Class]Hashtable::Remove which both assign null:
http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/xpcom/ds/nsRefPtrHashtable.h#187
same as the Get method does when an entry does not exist:
http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/xpcom/ds/nsRefPtrHashtable.h#116

Not assigning the outparam is fine with me, but we probably need to
do some auditing before changing those methods.  (Let me know if you
want me to file a follow-up bug on that.)

> I guess it saves a branch in some cases?

I doubt it matters for performance since these calls are inlined.
The vast majority of calls with an 'aData' argument use
getter_AddRefs(localVar) or &localVar so the assignment can
probably be optimized away (as redundant) in many cases, or if
the local variable isn't used after Remove/Get returns false
then the assignment can be optimized away for that reason.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed4185c7b73
part 1 - Unify the ns[Base|Interface|RefPtr]Hashtable::Remove() signatures for consistency.  Make it return true if an entry was removed, with an optional out param to move the value.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46d1d8506ad
part 2 - Use plain Remove(key) in some places instead of Lookup(key).Remove() for simplicity.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/29730c843219
part 3 - Make nsClassHashtable inherit Remove() for API consistency with ns[Base|Interface|RefPtr]Hashtable.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/512c69e35fc0
part 4 - Make all nsClassHashtable::RemoveAndForget() consumers use Remove() instead.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a27ab70450
part 5 - Remove nsClassHashtable::RemoveAndForget().  r=froydnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ed4185c7b73
https://hg.mozilla.org/mozilla-central/rev/d46d1d8506ad
https://hg.mozilla.org/mozilla-central/rev/29730c843219
https://hg.mozilla.org/mozilla-central/rev/512c69e35fc0
https://hg.mozilla.org/mozilla-central/rev/a4a27ab70450