1372317 - Consider adding a nsBaseHashtable::Lookup() method that allows modifying the value and remove the entry

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P4)

Description

[Mats Palmgren (inactive)]

Consider adding a nsBaseHashtable::Lookup() method that returns an EntrPtr
kind of object that you can use to read/write the value, and optionally
remove the entry.  To make it possible to write something like this:

if (auto entry = Lookup(key)) {
  // found it!
  ++entry.Data();
  if (entry.Data() == 42)
    entry.Remove();
}

We'd have to ensure that |entry| cannot be used after a Remove of course.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Introduce a nsBaseHashtable::Lookup() method that allows modifying the value and optionally remove the entry

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - Replace LookupRemoveIf() calls with Lookup() + entry.Remove() where needed

So, I started this part with the intention of replacing a few LookupRemoveIf()
calls where we didn't need the remove part, or where Lookup().Remove() would
be simpler.  I ended up replacing all calls.  There were a couple where
either solution would be fine, but I felt those didn't motivate keeping
LookupRemoveIf() around, so I think it'd be better to remove that method.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - Remove nsBaseHashtable::LookupRemoveIf() since it's not used anymore

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8878562 [details] [diff] [review]
part 1 - Introduce a nsBaseHashtable::Lookup() method that allows modifying the value and optionally remove the entry

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

::: xpcom/ds/nsBaseHashtable.h
@@ +226,5 @@
> +      mTable.RemoveEntry(mEntry);
> +      mEntry = nullptr;
> +    }
> +
> +    MOZ_MUST_USE DataType& Data()

I know other APIs (e.g. js::HashTable) would use operator* here instead of an explicitly named function.  WDYT about doing that instead?

@@ +229,5 @@
> +
> +    MOZ_MUST_USE DataType& Data()
> +    {
> +      MOZ_ASSERT(mTableGeneration == mTable.GetGeneration());
> +      return mEntry->mData;

Should we MOZ_ASSERT before accessing mEntry here, if only to provide a clearer error message?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8878565 [details] [diff] [review]
part 2 - Replace LookupRemoveIf() calls with Lookup() + entry.Remove() where needed

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

I think the control flow is clearer with Lookup() + entry.Remove(), thanks.  One problem noted below.

::: dom/base/ImageTracker.cpp
@@ +68,5 @@
> +    } else {
> +      return NS_OK;
> +    }
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("Removing image that wasn't in the tracker!");

In the (ideally nonexistent) case that we *do* remove an image that wasn't in the tracker, the new code will just fall through to the remainder of the function, since this assertion is not a release assertion, right?  That seems ungood, and contrary to the behavior we had before.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> > +    MOZ_MUST_USE DataType& Data()
> 
> I know other APIs (e.g. js::HashTable) would use operator* here instead of
> an explicitly named function.  WDYT about doing that instead?

Sure, I don't see a reason not to, but let's do that in a follow-up
bug and do the same for LookupForAdd in that case.  I filed bug 1374064.

> > +    MOZ_MUST_USE DataType& Data()
> > +    {
> > +      MOZ_ASSERT(mTableGeneration == mTable.GetGeneration());
> > +      return mEntry->mData;
> 
> Should we MOZ_ASSERT before accessing mEntry here, if only to provide a
> clearer error message?

Sure, I changed the assert above to:
      MOZ_ASSERT(!!*this, "must have an entry to access its value");

"operator bool" has the mTableGeneration check already.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> In the (ideally nonexistent) case that we *do* remove an image that wasn't
> in the tracker, the new code will just fall through to the remainder of the
> function, since this assertion is not a release assertion, right?  That
> seems ungood, and contrary to the behavior we had before.

Fair enough, I added an early return there.  I don't think falling through
would do any harm though.  It depends on the nature of the hypothetical bug
which path is less harmful I guess.

Anyway, at least it's better now than the original code which was to underflow
the zero count and then *add* the image to the table:
https://hg.mozilla.org/mozilla-central/annotate/3e153a5acd35/dom/base/ImageTracker.cpp#l57

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f418ab5447
part 1 - Introduce a nsBaseHashtable::Lookup() method that allows modifying the value and optionally remove the entry.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab565909b1d0
part 2 - Replace LookupRemoveIf() calls with Lookup() + entry.Remove() where needed.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2d83899477
part 3 - Remove nsBaseHashtable::LookupRemoveIf() since it's not used anymore.  r=froydnj

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55f418ab5447
https://hg.mozilla.org/mozilla-central/rev/ab565909b1d0
https://hg.mozilla.org/mozilla-central/rev/4e2d83899477