1361744 - Avoid a double hashtable lookup for insertions in ConnectionPool::Start

Status: RESOLVED WONTFIX

(Core :: Storage: IndexedDB, enhancement)

Description

[(no longer active)]

We can use the API added in bug 1359848 to be a bit smarter here.

Comment 1

[(no longer active)]

Attachment: Part 1: Add a couple of utility methods to nsClassHashtable::EntryPtr

Comment 2

[(no longer active)]

Attachment: Part 2: Avoid a double hashtable lookup for insertions in ConnectionPool::Start

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8864173 [details] [diff] [review]
Part 1: Add a couple of utility methods to nsClassHashtable::EntryPtr

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

Sure, why not.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Sure, why not.

Though I'm kind of skeptical that the EntryPtr is necessarily going to be valid for an extended period.  I know this makes things convenient, but maybe it'd be better to just fetch the value out of the entry after Insert(), rather than trying to make the EntryPtr into something it's not?

Comment 5

[(no longer active)]

Comment on attachment 8864173 [details] [diff] [review]
Part 1: Add a couple of utility methods to nsClassHashtable::EntryPtr

Nathan and I talked about this on IRC.  He asked me to instead use operator*() to deref these EntryPtr objects and assign the result to normal pointers and use them.

Comment 6

[(no longer active)]

Attachment: Avoid a double hashtable lookup for insertions in ConnectionPool::Start

Comment 7

[(no longer active)]

Comment on attachment 8864263 [details] [diff] [review]
Avoid a double hashtable lookup for insertions in ConnectionPool::Start

I think this API is all gonna change in bug 1361745, it's probably not worth your time looking at this patch yet Bevis.  Sorry for the noise...

Comment 8

[(no longer active)]

Attachment: Avoid a double hashtable lookup for insertions in ConnectionPool::Start

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8865074 [details] [diff] [review]
Avoid a double hashtable lookup for insertions in ConnectionPool::Start

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

::: dom/indexedDB/ActorsParent.cpp
@@ +12411,1 @@
>      MutexAutoLock lock(mDatabasesMutex);

There is a trade-off here with the new OrInsert() method to always lock the mutex which is not necessary when the dbInfo is not new.

Lock the mutex only on all the modification or the reading off the owning thread:
http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/dom/indexedDB/ActorsParent.cpp#5459-5462

Comment 10

[(no longer active)]

Hmm, yeah that is a good point.

Nathan made me remove the ability to read the looked up pointer directly without calling OrInsert() in bug 1361745, which means that this API is not useful for this call site.  That's unfortunate, but the cost of extra locking is probably a lot worse than the cost of the extra function lookup.  Maybe some day he will be convinced to add the deref operator back to EntryPtr and then we can do this again.  As things stand, I don't see a way to fix this bug.