| Summary: | IndexedDB: Use WeakPtr for Factory-to-BackingStore reference | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||||
| Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, alecflett, dgrogan, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | 111819 | ||||||||||||||
| Bug Blocks: | 110820 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Joshua Bell
2013-03-05 11:12:26 PST
Created attachment 191519 [details]
Patch
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 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.)
Created attachment 191803 [details]
Patch
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 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. Created attachment 192086 [details]
Patch
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. (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. (In reply to comment #9) > It could be re-used "it" = WeakPtr + HashMap > ...to the need for... ...to replace... Comment on attachment 192086 [details] Patch Clearing flags on attachment: 192086 Committed r145166: <http://trac.webkit.org/changeset/145166> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 111819 Created attachment 192239 [details]
Patch for landing
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 Created attachment 192241 [details]
Patch for landing
(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 on attachment 192241 [details] Patch for landing Clearing flags on attachment: 192241 Committed r145238: <http://trac.webkit.org/changeset/145238> All reviewed patches have been landed. Closing bug. |