Closed Bug 1359848 Opened 3 years ago Closed 3 years ago

nsPrefBranch::AddObserver() should only do one hashtable lookup in mObservers in case where a new callback is being set

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(3 files)

Right now it's doing two because nsClassHashtable doesn't have the right API for the use case of "add an entry to the hashtable if one doesn't exist but please tell me if you did that."
FWIW this has come up in real profiles like bug 1350745.
See Also: → 1350745
This sort of API is something that I've wanted to add for a while.  My first instinct is to base things off js::Hash$FOO::lookupForAdd:

http://dxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#117

without dealing with relookupOrAdd.  So the API in the AddObserver case would be used like:

  // Would need to qualify EntryPtr somehow or use `auto`.
  EntryPtr p = mObservers.LookupForAdd(pCallback);
  if (p) {
    // Entry already exists
    NS_WARNING(...);
    delete pCallback;
    return NS_OK;
  }

  // Insert into our table.  Not sure of the name here, but want to avoid using operator*
  // here so we can assert in operator* that the entry has something in it.  Alternatively,
  // this could be nsClassHashtable::Insert(EntryPtr&, UserDataType*).
  p.TakeOwnership(pCallback);

Some thoughts about the EntryPtr implementation:

* Should ensure that the table hasn't been modified by recording the generation of the table when the EntryPtr is created.
* Should assert at destruction that we've used it to insert into the table.

WDYT?
Flags: needinfo?(ehsan)
I mentioned this to you on IRC today but I think you missed it.  I was hoping you would ask for something other than changing the signature of nsClassHashtable::LookupOrAdd() to avoid having to also fix all of those call sites here.  :/  Any chance we could defer that to some other bug?

(In reply to Nathan Froyd [:froydnj] from comment #2)
> This sort of API is something that I've wanted to add for a while.  My first
> instinct is to base things off js::Hash$FOO::lookupForAdd:
> 
> http://dxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#117
> 
> without dealing with relookupOrAdd.  So the API in the AddObserver case
> would be used like:
> 
>   // Would need to qualify EntryPtr somehow or use `auto`.
>   EntryPtr p = mObservers.LookupForAdd(pCallback);
>   if (p) {
>     // Entry already exists
>     NS_WARNING(...);
>     delete pCallback;
>     return NS_OK;
>   }
> 
>   // Insert into our table.  Not sure of the name here, but want to avoid
> using operator*
>   // here so we can assert in operator* that the entry has something in it. 
> Alternatively,
>   // this could be nsClassHashtable::Insert(EntryPtr&, UserDataType*).
>   p.TakeOwnership(pCallback);
> 
> Some thoughts about the EntryPtr implementation:
> 
> * Should ensure that the table hasn't been modified by recording the
> generation of the table when the EntryPtr is created.
> * Should assert at destruction that we've used it to insert into the table.

I'm not really following the suggestion yet...  Just so that we're both on the same page, by EntryPtr do you mean base_type::EntryType here for example? <http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/xpcom/ds/nsClassHashtable.h#83>  If yes, I can't really parse the above.  But I suspect you may be referring to something else?

The rough idea that I had in my mind was an API like this:

  MOZ_MUST_USE bool AddIfNotExists(KeyType aKey, const UserDataType&);

It will simply return true if the entry was inserted into the hashtable and false otherwise...
Flags: needinfo?(ehsan) → needinfo?(nfroyd)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #3)
> I mentioned this to you on IRC today but I think you missed it.  I was
> hoping you would ask for something other than changing the signature of
> nsClassHashtable::LookupOrAdd() to avoid having to also fix all of those
> call sites here.  :/  Any chance we could defer that to some other bug?

Which is why my proposed function is called LookupForAdd, not LookupOrAdd. :)  It's not great to have two functions with such similar names, but there you go.

> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > This sort of API is something that I've wanted to add for a while.  My first
> > instinct is to base things off js::Hash$FOO::lookupForAdd:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#117
> > 
> > without dealing with relookupOrAdd.  So the API in the AddObserver case
> > would be used like:
> > 
> >   // Would need to qualify EntryPtr somehow or use `auto`.
> >   EntryPtr p = mObservers.LookupForAdd(pCallback);
> >   if (p) {
> >     // Entry already exists
> >     NS_WARNING(...);
> >     delete pCallback;
> >     return NS_OK;
> >   }
> > 
> >   // Insert into our table.  Not sure of the name here, but want to avoid
> > using operator*
> >   // here so we can assert in operator* that the entry has something in it. 
> > Alternatively,
> >   // this could be nsClassHashtable::Insert(EntryPtr&, UserDataType*).
> >   p.TakeOwnership(pCallback);
> > 
> > Some thoughts about the EntryPtr implementation:
> > 
> > * Should ensure that the table hasn't been modified by recording the
> > generation of the table when the EntryPtr is created.
> > * Should assert at destruction that we've used it to insert into the table.
> 
> I'm not really following the suggestion yet...  Just so that we're both on
> the same page, by EntryPtr do you mean base_type::EntryType here for
> example?
> <http://searchfox.org/mozilla-central/rev/
> ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/xpcom/ds/nsClassHashtable.h#83>  If
> yes, I can't really parse the above.  But I suspect you may be referring to
> something else?

My idea was something like, as an inner class of nsClassHashtable:

struct EntryPtr {
  base_type::EntryType& mEntry;
  // for debugging purposes
#ifdef DEBUG
  uint32_t mTableGeneration;
#endif

  explicit operator bool() const
  {
    // Is there something stored in the table already?
    return !!mEntry.mData;
  }

  T& operator*()
  {
    MOZ_ASSERT(mEntry.mData);
    return *mData;
  }

  void TakeOwnership(T* aPtr)
  {
    MOZ_ASSERT(!mEntry.mData);
    mEntry.mData = aPtr;
  }
};

...with perhaps a few other modifications.  Does that sound any more attractive?
Flags: needinfo?(nfroyd)
Yes, I think this idea makes sense now, sorry I missed the "For" in the name and totally confused myself initially.  :-)

I'm writing a patch.
Comment on attachment 8862965 [details] [diff] [review]
Part 2: Avoid two hashtable lookups when registering a new preference observer

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

r=me with the corresponding changes from the API differences in part 1.
Attachment #8862965 - Flags: review?(nfroyd) → review+
Comment on attachment 8862964 [details] [diff] [review]
Part 1: Add the nsClassHashtable::LookupForAdd() API to allow consumers to lookup and add an entry to a class hashtable if it doesn't exist already with a single lookup

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

r=me with the API changes below.

::: xpcom/ds/nsClassHashtable.h
@@ +105,5 @@
> +      MOZ_ASSERT(mTableGeneration == mTable.GetGeneration());
> +      return *mEntry.mData;
> +    }
> +
> +    void TakeOwnership(T* aPtr)

On second thought, let's make this an API on nsTHashtable, so we'd have:

auto p = table.LookupForAdd(key);
if (!p) {
  table.Insert(p, newValue);
}

@@ +126,5 @@
> +   *     // The entry already existed in the table.
> +   *     Use(*p);
> +   *   } else {
> +   *     // An existing entry wasn't found, store a new entry in the hashtable.
> +   *     p.TakeOwnership(newValue);

This comment (which is great, btw!) would need to be modified for the new API.
Attachment #8862964 - Flags: review?(nfroyd) → review+
Yeah good idea, there is really no reason to not have it in nsTHashtable.
I'm having difficulty figuring out how to do this in nsTHashtable properly.  The problem is that nsBaseHashtable doesn't inherit nsTHashtable's API, and doing this would require me to reimplement the Insert() method in every leaf class of nsBaseHashtable as far as I can tell.  Is this really what you wanted me to do?
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #12)
> I'm having difficulty figuring out how to do this in nsTHashtable properly. 
> The problem is that nsBaseHashtable doesn't inherit nsTHashtable's API, and
> doing this would require me to reimplement the Insert() method in every leaf
> class of nsBaseHashtable as far as I can tell.  Is this really what you
> wanted me to do?

Ugh.  I wrote nsTHashtable, but what I really meant was nsClassHashtable.  We can't really do it on nsTHashtable, because there's no concept of "value" there.  Putting it on nsBaseHashtable would be ideal, since we have the "value" concept, but I don't think there's a good way to express it such that it gets the right semantics for all nsBaseHashtable subclasses.

Let's put it on nsClassHashtable for now, and we can work at unifying the API if necessary at a later point.  Sound good?
Flags: needinfo?(nfroyd)
Sure, that sounds good too!  I still wish doing a more generic API on nsBaseHashtable would have been simpler!  :-)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9f8d4e4d74
Part 1: Add the nsClassHashtable::LookupForAdd() API to allow consumers to lookup and add an entry to a class hashtable if it doesn't exist already with a single lookup; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dffad00f63
Part 2: Avoid two hashtable lookups when registering a new preference observer; r=froydnj
https://hg.mozilla.org/mozilla-central/rev/df9f8d4e4d74
https://hg.mozilla.org/mozilla-central/rev/c2dffad00f63
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.