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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

This intends to expose basically the same functionality as operator[] on
unordered_map.

Comment 1

[Trevor Saunders (:tbsaunde)]

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

This intends to expose basically the same functionality as operator[] on
unordered_map.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Trevor Saunders (:tbsaunde)]

(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.

Comment 4

[Nathan Froyd [:froydnj]]

(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.

Comment 5

[Nathan Froyd [:froydnj]]

Alternatively, why not keep the API in nsBaseHashtable, but have it return a DataType&?

Comment 6

[Trevor Saunders (:tbsaunde)]

(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 ;)

Comment 7

[Trevor Saunders (:tbsaunde)]

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

This intends to expose basically the same functionality as operator[] on
unordered_map.

Comment 8

[Nathan Froyd [:froydnj]]

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.

Comment 9

[Trevor Saunders (:tbsaunde)]

> ::: 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 :(

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: add a GetOrInsert to nsBaseHashtable that returns a reference to the value

This intends to expose basically the same functionality as operator[] on
unordered_map.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e27121698e4
add a GetOrInsert to nsBaseHashtable that returns a reference to the value r=froydnj

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c428116222e7
Backed out changeset 0e27121698e4 for a11y failures a=backout CLOSED TREE

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e943ad944554
add a GetOrInsert to nsBaseHashtable that returns a reference to the value r=froydnj

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8811465 [details] [diff] [review]
add a GetOrInsert to nsBaseHashtable that returns a reference to the value

dep of bug 1270916

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fe631df8c66