If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
P4
enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → mats
(Assignee)

Comment 1

4 months ago
Created attachment 8878562 [details] [diff] [review]
part 1 - Introduce a nsBaseHashtable::Lookup() method that allows modifying the value and optionally remove the entry
Attachment #8878562 - Flags: review?(nfroyd)
(Assignee)

Comment 2

4 months ago
Created attachment 8878565 [details] [diff] [review]
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.
Attachment #8878565 - Flags: review?(nfroyd)
(Assignee)

Comment 3

4 months ago
Created attachment 8878566 [details] [diff] [review]
part 3 - Remove nsBaseHashtable::LookupRemoveIf() since it's not used anymore
Attachment #8878566 - Flags: review?(nfroyd)
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?
Attachment #8878562 - Flags: review?(nfroyd) → review+
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.
Attachment #8878565 - Flags: review?(nfroyd) → review+

Updated

4 months ago
Attachment #8878566 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 6

4 months ago
(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.
(Assignee)

Comment 7

4 months ago
(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

4 months ago
Pushed by mpalmgren@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55f418ab5447
https://hg.mozilla.org/mozilla-central/rev/ab565909b1d0
https://hg.mozilla.org/mozilla-central/rev/4e2d83899477
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

4 months ago
Blocks: 1376126
(Assignee)

Updated

4 months ago
Blocks: 1376293
(Assignee)

Updated

4 months ago
Blocks: 1376296
(Assignee)

Updated

4 months ago
Blocks: 1376297
(Assignee)

Updated

4 months ago
Blocks: 1376460
(Assignee)

Updated

4 months ago
Blocks: 1376463
(Assignee)

Updated

4 months ago
Blocks: 1376468
(Assignee)

Updated

4 months ago
Blocks: 1376473
(Assignee)

Updated

4 months ago
Blocks: 1376476
(Assignee)

Updated

4 months ago
Blocks: 1376477
(Assignee)

Updated

4 months ago
Blocks: 1376483
(Assignee)

Updated

4 months ago
Blocks: 1376491
You need to log in before you can comment on or make changes to this bug.