add a GetOrInsert to nsbaseHashtable that returns a reference to the value

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This intends to expose basically the same functionality as operator[] on
unordered_map.
This intends to expose basically the same functionality as operator[] on
unordered_map.
Attachment #8808792 - Flags: review?(nfroyd)
Comment on attachment 8808792 [details] [diff] [review]
add a GetOrInsert to nsbaseHashtable that returns a reference to the value

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

I don't think this is correct for things like nsRefPtrHashtable, where you're trying to return a reference to a raw pointer.

If we want to provide something like this, it'd be *really* nice if we weren't doing two hashtable lookups for the insert case, but reusing the slot we tried to find earlier.
Attachment #8808792 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8808792 [details] [diff] [review]
> add a GetOrInsert to nsbaseHashtable that returns a reference to the value
> 
> Review of attachment 8808792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is correct for things like nsRefPtrHashtable, where
> you're trying to return a reference to a raw pointer.

I think that fails to compile because ent->mData is a RefPtr and you can get a Foo* out of that, but you can't convert it to a reference.  A quick test seems to confirm that, but there may be some other case where this doesn't work.

That said I'd be fine with moving this to nsDataHashtable.

> If we want to provide something like this, it'd be *really* nice if we
> weren't doing two hashtable lookups for the insert case, but reusing the
> slot we tried to find earlier.

True, but I don't see an easy way to do that with PLDHash, and I don't really have time at the moment to deal with that :/  That said this seems preferable to the alternative of call twice manually, at least this way when we have time to fix PLDHash we can get all the users for free.

I guess I could rewrite the code that needs this to use nsTHashtable directly and build my own mapping stuff, but I'd rather not.
Flags: needinfo?(nfroyd)
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > I don't think this is correct for things like nsRefPtrHashtable, where
> > you're trying to return a reference to a raw pointer.
> 
> I think that fails to compile because ent->mData is a RefPtr and you can get
> a Foo* out of that, but you can't convert it to a reference.  A quick test
> seems to confirm that, but there may be some other case where this doesn't
> work.
> 
> That said I'd be fine with moving this to nsDataHashtable.

I guess I'd be OK with that.

> > If we want to provide something like this, it'd be *really* nice if we
> > weren't doing two hashtable lookups for the insert case, but reusing the
> > slot we tried to find earlier.
> 
> True, but I don't see an easy way to do that with PLDHash, and I don't
> really have time at the moment to deal with that :/  That said this seems
> preferable to the alternative of call twice manually, at least this way when
> we have time to fix PLDHash we can get all the users for free.

Yeah.
Flags: needinfo?(nfroyd)
Alternatively, why not keep the API in nsBaseHashtable, but have it return a DataType&?
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Alternatively, why not keep the API in nsBaseHashtable, but have it return a
> DataType&?

yes, returning a reference to the actual type in memory makes a lot of sense ;)
This intends to expose basically the same functionality as operator[] on
unordered_map.
Attachment #8811026 - Flags: review?(nfroyd)
Comment on attachment 8811026 [details] [diff] [review]
add a GetOrInsert to nsbaseHashtable that returns a reference to the value

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

Please fix the commit message to correctly capitalize nsBaseHashtable.

::: xpcom/glue/nsBaseHashtable.h
@@ +126,5 @@
> +      return ent->mData;
> +    }
> +
> +    ent = this->PutEntry(aKey);
> +    new(&ent->mData) UserDataType();

This seems unnecessary, as PutEntry() will have called EntryType::EntryType() on the entry already, which will have default-constructed |mData|.  So I think this can be removed.
Attachment #8811026 - Flags: review?(nfroyd) → feedback+
Attachment #8808792 - Attachment is obsolete: true
> ::: xpcom/glue/nsBaseHashtable.h
> @@ +126,5 @@
> > +      return ent->mData;
> > +    }
> > +
> > +    ent = this->PutEntry(aKey);
> > +    new(&ent->mData) UserDataType();
> 
> This seems unnecessary, as PutEntry() will have called
> EntryType::EntryType() on the entry already, which will have
> default-constructed |mData|.  So I think this can be removed.

yeah, I really should have caught that, sorry :(
This intends to expose basically the same functionality as operator[] on
unordered_map.
Attachment #8811465 - Flags: review?(nfroyd)
Attachment #8811026 - Attachment is obsolete: true
Attachment #8811465 - Flags: review?(nfroyd) → review+
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e27121698e4
add a GetOrInsert to nsBaseHashtable that returns a reference to the value r=froydnj
This (or bug 127-016) appears to have caused the a11y tests to fail like https://treeherder.mozilla.org/logviewer.html#?job_id=39326093&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b9d24cdb97
Flags: needinfo?(tbsaunde+mozbugs)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c428116222e7
Backed out changeset 0e27121698e4 for a11y failures a=backout CLOSED TREE
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e943ad944554
add a GetOrInsert to nsBaseHashtable that returns a reference to the value r=froydnj
https://hg.mozilla.org/mozilla-central/rev/e943ad944554
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8811465 [details] [diff] [review]
add a GetOrInsert to nsBaseHashtable that returns a reference to the value

dep of bug 1270916
Attachment #8811465 - Flags: approval-mozilla-aurora?
Comment on attachment 8811465 [details] [diff] [review]
add a GetOrInsert to nsBaseHashtable that returns a reference to the value

OK for aurora, part of e10s/a11y uplift along with work from bug 1270916.
Attachment #8811465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → tbsaunde+mozbugs
Depends on: 1371061
You need to log in before you can comment on or make changes to this bug.