Bug 111459 - IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
Summary: IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on: 111819
Blocks: 110820
  Show dependency treegraph
 
Reported: 2013-03-05 11:12 PST by Joshua Bell
Modified: 2013-03-11 13:29 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.15 KB, patch)
2013-03-05 11:18 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (14.11 KB, patch)
2013-03-06 11:45 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (16.44 KB, patch)
2013-03-07 14:43 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (17.27 KB, patch)
2013-03-08 09:34 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (17.26 KB, patch)
2013-03-08 09:44 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2013-03-05 11:12:26 PST
IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
Comment 1 Joshua Bell 2013-03-05 11:18:28 PST
Created attachment 191519 [details]
Patch
Comment 2 Joshua Bell 2013-03-05 11:29:25 PST
This is an exploratory change. We should probably not use it as-is.

...

As currently implemented, the factory maintains a map of origin->backing store (raw ptr) so that each database opened within the same origin uses the same backing store. To maintain this map, the backing store holds a RefPtr to the factory and when it is destructed it calls into the factory to remove the map entry.

This patch changes the map's pointer type to a WeakRef, so that backing store no longer needs an explicit reference to the factory.

The impetus is https://bugs.webkit.org/show_bug.cgi?id=110820 - for in-memory backing stores, we want the IDBFactoryBackendImpl lifetime to dictate the IDBBackingStore lifetime - so, the factory should maintain a RefPtr to the backing store. If the backing store had a RefPtr to the factory that would introduce a reference cycle.

...

As written the HashMap<String, WeakPtr<>> entries will never be cleared out. That will cost some memory for each origin encountered, which is not ideal. We may need to introduce a WeakPtrHashMap<> that cleans up HashMap entries when the ref goes away.

...

Thoughts?
Comment 3 Adam Barth 2013-03-05 13:17:12 PST
Comment on attachment 191519 [details]
Patch

Sounds like a reasonable idea.  We're trying to be careful about where we use WeakPtr so that we gain experience with it slowly, but this sounds like a good use case.  (I haven't reviewed the details of this patch.)
Comment 4 Joshua Bell 2013-03-06 11:45:41 PST
Created attachment 191803 [details]
Patch
Comment 5 Joshua Bell 2013-03-06 11:49:24 PST
Rebased on other IDBBackingStore changes.

This prevents the HashMap<K,WeakPtr<M>> from growing unbounded by introducing an explicit "cleanWeakMap" call at the point of insertion that scrubs the map of nulled WeakPtrs. Not an ideal approach, but plausible.
Comment 6 Adam Barth 2013-03-06 14:42:12 PST
Comment on attachment 191803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191803&action=review

We could also let you register a callback with WeakPtr to be notified when it gets cleared, but we probably don't need to tech to that level yet.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:45
> +template<typename K, typename M>
> +static void cleanWeakMap(HashMap<K, WeakPtr<M> >& map)

This seems like a pretty general-purpose function for this file.  I wonder if we should move it to WTF somewhere...  Maybe this pattern is obscure enough that it's fine to keep it here.
Comment 7 Joshua Bell 2013-03-07 14:43:59 PST
Created attachment 192086 [details]
Patch
Comment 8 Joshua Bell 2013-03-07 14:45:20 PST
alecflett@ - can you look this over before landing since the merge with your backing store changes/tests involved deleting some assertions? I may have missed the intent of something.
Comment 9 Joshua Bell 2013-03-07 15:54:52 PST
(In reply to comment #6)
> This seems like a pretty general-purpose function for this file.  I wonder if we should move it to WTF somewhere...  Maybe this pattern is obscure enough that it's fine to keep it here.

Yeah, I coded it in a generic fashion anticipating re-use. It could be re-used in the following other places in IDB to the need for custom "lifetime awareness" tracking:

IDBTransaction - replace register/unregisterOpenCursor/HashSet<IDBCursor*>
IDBTransactionBackendImpl - same thing, but the back end objects
IDBFactoryBackendImpl - currently manages lifetime of IDBDatabaseBackendImpls but could be made weak

Given that it bridges HashSet and WeakPtr I didn't spot a good place in WTF to wedge it, but will keep pondering.
Comment 10 Joshua Bell 2013-03-07 15:56:15 PST
(In reply to comment #9)
> It could be re-used

"it" = WeakPtr + HashMap

> ...to the need for...

...to replace...
Comment 11 WebKit Review Bot 2013-03-07 17:44:22 PST
Comment on attachment 192086 [details]
Patch

Clearing flags on attachment: 192086

Committed r145166: <http://trac.webkit.org/changeset/145166>
Comment 12 WebKit Review Bot 2013-03-07 17:44:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2013-03-07 21:44:08 PST
Re-opened since this is blocked by bug 111819
Comment 14 Joshua Bell 2013-03-08 09:34:28 PST
Created attachment 192239 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-03-08 09:41:12 PST
Comment on attachment 192239 [details]
Patch for landing

Rejecting attachment 192239 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 192239, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17109112
Comment 16 Joshua Bell 2013-03-08 09:44:43 PST
Created attachment 192241 [details]
Patch for landing
Comment 17 Joshua Bell 2013-03-08 09:45:53 PST
(In reply to comment #15)
> /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Apparently my "update changelogs with newly added files" script has some glitches. :P
Comment 18 WebKit Review Bot 2013-03-08 10:08:30 PST
Comment on attachment 192241 [details]
Patch for landing

Clearing flags on attachment: 192241

Committed r145238: <http://trac.webkit.org/changeset/145238>
Comment 19 WebKit Review Bot 2013-03-08 10:08:36 PST
All reviewed patches have been landed.  Closing bug.