Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2314933005:
Align IndexedDB metadata rollback on transaction abort to spec. (Closed)
DescriptionAlign IndexedDB metadata rollback on transaction abort to spec.
When an IndexedDB versionchange transaction is aborted, Blink's
IndexedDB metadata is not entirely reverted, causing our behavior to
deviate from the IndexedDB specification in minor, subtle ways. This
change aligns our behavior with the spec and with Firefox's
implementation.
The change also (slightly) reduces IndexedDB memory usage in two ways.
(1) Object store and index metadata is now shared between an IDBDatabase
and its associated IDBObjectStore and IDBIndex instances, instead of
being copied. (2) versionchange transactions only back up the metadata
for object stores that are accessed by JavaScript, instead of creating a
backup for the entire database metadata.
BUG=645018, 457447
Committed: https://crrev.com/3572fef3913166c5da29dca38c04bf6f6683560d
Cr-Commit-Position: refs/heads/master@{#423742}
Patch Set 1 : Add failing tests. #Patch Set 2 : Uncommented tests that Firefox fails. #Patch Set 3 : Rebased. #
Total comments: 14
Patch Set 4 : Addressed feedback, removed Own types. (take 2) #Patch Set 5 : CL for src perf tryjob to run storage.indexeddb_endure_tracing benchmark on mac-retina platform(s) #Patch Set 6 : CL for src perf tryjob to run storage.indexeddb_endure_tracing benchmark on linux platform(s) #Patch Set 7 : Rebased. #
Total comments: 28
Patch Set 8 : Documented tests that currently fail in Firefox. #Patch Set 9 : Addressed some feedback. #Patch Set 10 : Removed test TODOs, because Firefox nightly passes all the tests. #Patch Set 11 : Rebased past the big reformat. #
Total comments: 28
Patch Set 12 : Rebased #Patch Set 13 : Addressed feedback. #
Total comments: 16
Patch Set 14 : Addressed feedback. #Messages
Total messages: 104 (83 generated)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Description was changed from ========== Refactor IndexedDB transaction rollback metadata. BUG= ========== to ========== Refactor IndexedDB transaction rollback metadata. This refactoring fixes the way Blink's IndexedDB metadata is reverted when a versionchange transaction is aborted. It also reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:180001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:220001) has been deleted
Patchset #2 (id:200001) has been deleted
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
PTAL.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
test l*g*t*m I'm still working my way through the cpp/h changes. The introduction of the "Own" types seems unfortunate. I wonder if there's a way to collapse them. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js:85: const request = indexedDB.open(databaseName(testCase), version); Maybe add onupgradeneeded = testCase.unreached_func(' ... '); ? https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:339: const IDBObjectStoreOwnMetadata& metadata = objectStore->metadata().own; It seems wrong for IDBCursor to know so much about how we manage the metadata. metadata.autoIncrement() could just be objectStore->autoIncrement(); metadata. We could add an IDBObjectStore::idbKeyPath() accessor too. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:66: IDBObjectStoreOwnMetadata& operator =(const IDBObjectStoreOwnMetadata&); nit: we usually write: operator= https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:61: static IDBTransaction* createNonVersionChange(ScriptState*, int64_t, const HashSet<String>& objectStoreNames, WebIDBTransactionMode, IDBDatabase*); Why these renames? If it's just to avoid the overload, I'd keep this one just "create". https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:90: // database's metadata. Can you expand this comment to answer "why?" https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:161: // The cache may be in an inconsistent state after a transaction is aborted. I think this can be simplified to e.g.: // objectStore() throws for completed/aborted transactions, // so this is not used after that occurs and can be cleared. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:172: // The metadata of deleted object stores without IDBObjectStore instaces. typo: instances
jsbell: Thank you very much for the quick initial feedback! Regarding the Own datatypes -- I'll try to figure out a way to collapse them. I think I can do that at the cost of a tiny bit of memory. Do you agree with turning the metadata structs into RefPtr graphs to save memory, or did you dislike that as well? Asking because if you don't like this, I'd like to do a single revision pass for both structs and Own datatypes. Thank you!
On 2016/09/16 20:03:56, pwnall wrote: > Do you agree with turning the metadata structs into RefPtr graphs to save > memory, or did you dislike that as well? Asking because if you don't like this, > I'd like to do a single revision pass for both structs and Own datatypes. sgtm. (I was assuming you were trying to maintain the "Own" types just to simplify object lifetimes in the basic case, but I think it makes the code less maintainable)
On 2016/09/16 20:25:21, jsbell wrote: > On 2016/09/16 20:03:56, pwnall wrote: > > Do you agree with turning the metadata structs into RefPtr graphs to save > > memory, or did you dislike that as well? Asking because if you don't like > this, > > I'd like to do a single revision pass for both structs and Own datatypes. > > sgtm. (I was assuming you were trying to maintain the "Own" types just to > simplify object lifetimes in the basic case, but I think it makes the code less > maintainable) To clarify: "turning the metadata structs into RefPtr graphs to save memory" sgtm if that lets us consolidate the types. Relative to e.g. nodes, even the most extreme cases would have only 1000s of store/index object around, so we're not producing that much memory. Obviously if they're refptrs we need to watch out for cycles. In these cases I assume it's going to be a pretty simple "unidirectional" graph so no risk of cycles.
Patchset #4 (id:280001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell: Thank you for helping me make this code better! I addressed all the feedback except for removing the Own structs. I'll do that in a separate versions so the two versions can be compared. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js:85: const request = indexedDB.open(databaseName(testCase), version); On 2016/09/16 18:17:28, jsbell wrote: > Maybe add onupgradeneeded = testCase.unreached_func(' ... '); ? I thought EventWatcher is supposed to reject its promise if it receives any event that doesn't match the expectation list. [1] says "EventWatcher will assert if an event occurs while there is no wait_for() created Promise waiting to be fulfilled, or if the event is of a different type to the type currently expected. This ensures that only the events that are expected occur, in the correct order, and with the correct timing." [1] https://github.com/w3c/testharness.js/blob/master/docs/api.md https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:339: const IDBObjectStoreOwnMetadata& metadata = objectStore->metadata().own; On 2016/09/16 18:17:28, jsbell wrote: > It seems wrong for IDBCursor to know so much about how we manage the metadata. > > metadata.autoIncrement() could just be objectStore->autoIncrement(); > metadata. We could add an IDBObjectStore::idbKeyPath() accessor too. Done. This looks so clean! :) https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:66: IDBObjectStoreOwnMetadata& operator =(const IDBObjectStoreOwnMetadata&); On 2016/09/16 18:17:28, jsbell wrote: > nit: we usually write: operator= Done. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:61: static IDBTransaction* createNonVersionChange(ScriptState*, int64_t, const HashSet<String>& objectStoreNames, WebIDBTransactionMode, IDBDatabase*); On 2016/09/16 18:17:29, jsbell wrote: > Why these renames? > > If it's just to avoid the overload, I'd keep this one just "create". The renames document when each create version gets called. If you don't like the "createNonVersionChange" name, I can move that information into a comment with an accompanying DCHECK. Does this seem reasoanble? https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:90: // database's metadata. On 2016/09/16 18:17:29, jsbell wrote: > Can you expand this comment to answer "why?" Done. Can you please see if my revision makes sense? https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:161: // The cache may be in an inconsistent state after a transaction is aborted. On 2016/09/16 18:17:29, jsbell wrote: > I think this can be simplified to e.g.: > > // objectStore() throws for completed/aborted transactions, > // so this is not used after that occurs and can be cleared. > > Done. I also revised the comment on IDBIndex. https://codereview.chromium.org/2314933005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:172: // The metadata of deleted object stores without IDBObjectStore instaces. On 2016/09/16 18:17:29, jsbell wrote: > typo: instances Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:300001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:320001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
CL for src perf tryjob to run storage.indexeddb_endure_tracing benchmark on mac-retina platform(s)
Patchset #4 (id:340001) has been deleted
CL for src perf tryjob to run storage.indexeddb_endure_tracing benchmark on linux platform(s)
Patchset #7 (id:420001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some nits, some requests for comments, and some concerns about strategy. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:106: using ObjectStoreMap = HashMap<int64_t, RefPtr<IDBObjectStoreMetadata>>; Nit: Per style guide (google, not overridden by chromium or blink), usings/typedefs go at the start of a public (etc) section of the definition. https://google.github.io/styleguide/cppguide.html#Declaration_Order May want to re-order in the above classes too. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:58: Nit: remove extra blank line https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:851: // its metadata will remain as-is. We just need to look up the You can drop the second sentence here ("We just need...") as that describes what the code does, not why. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:860: // unconditionally reset the deletion marker. We could find out if the I think you can drop the alternative here ("We could...") https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:112: /// finishes. The exception is stores that are created and deleted in the Nit: extra / https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:201: DCHECK(m_database->metadata().objectStores.contains(objectStoreId)); Can you add a comment about what it means when the name is not found? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:241: const auto& objectStoreIterator = m_oldStoreMetadata.find(objectStore); Can you add a comment about what this case means? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:246: DCHECK(oldStoreMetadata); Can you add a comment about what this case means? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:511: // Remove references to the IDBObjectStore and IDBIndex instances held by This comment is misleading; they types can be GC'd even without this since there should be no external references and Oilpan understands cycles. We also shouldn't refer to the project name ("Oilpan") itself in the comments. Per comment in the old code, in theory the rest of this method (from here down) can be deleted now. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:91: // implementations need to access the metadata values before the change. "some implementations" sounds odd; does this mean tests? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:122: int64_t oldMaxObjectStoreId() const This is odd data to expose just for this use case, and means the logic is fragile (e.g. if we switched IDs to be UUIDs instead or something). Can this be redone as e.g. a "isNewlyCreated()" method that does the checks internally, or even just holds a flag? I'm not positive that's a huge improvement, though. Perhaps (per below), each store/index holds its own m_oldMetadata member; if that's null then the store/index is newly created in this transaction. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:194: HeapHashMap<Member<IDBObjectStore>, RefPtr<IDBObjectStoreMetadata>> m_oldStoreMetadata; What do you think about storing the data on the stores themselves, e.g. in an m_oldMetadata member? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:215: // Shallow snapshot of the database metadata when the transaction start. nit: wording; should that be "starts" ?
Description was changed from ========== Refactor IndexedDB transaction rollback metadata. This refactoring fixes the way Blink's IndexedDB metadata is reverted when a versionchange transaction is aborted. It also reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ========== to ========== Align IndexedDB metadata rollback on transaction abort to spec. When an IndexedDB versionchange transaction is aborted, Blink's IndexedDB metadata is not entirely reverted, causing our behavior to deviate from the IndexedDB specification in minor, subtle ways. This change aligns our behavior with the spec and with Firefox's implementation. The change also (slightly) reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell: Thank you for going through the code so quickly after you got back! Can you please take a look at my responses and see what you think? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:106: using ObjectStoreMap = HashMap<int64_t, RefPtr<IDBObjectStoreMetadata>>; On 2016/09/26 22:23:40, jsbell wrote: > Nit: Per style guide (google, not overridden by chromium or blink), > usings/typedefs go at the start of a public (etc) section of the definition. > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > May want to re-order in the above classes too. Done. I also moved "using" in IDBObjectStore.h and IDBTransaction.h https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:58: On 2016/09/26 22:23:40, jsbell wrote: > Nit: remove extra blank line Done. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:851: // its metadata will remain as-is. We just need to look up the On 2016/09/26 22:23:40, jsbell wrote: > You can drop the second sentence here ("We just need...") as that describes what > the code does, not why. Done. Thanks, that's a very good point! https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:860: // unconditionally reset the deletion marker. We could find out if the On 2016/09/26 22:23:40, jsbell wrote: > I think you can drop the alternative here ("We could...") Done. Sure, as long as one of us remembers why I wrote it like that :D https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:112: /// finishes. The exception is stores that are created and deleted in the On 2016/09/26 22:23:40, jsbell wrote: > Nit: extra / Done. https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:201: DCHECK(m_database->metadata().objectStores.contains(objectStoreId)); On 2016/09/26 22:23:40, jsbell wrote: > Can you add a comment about what it means when the name is not found? Done. Thank you! I think future me will be really grateful to you for this suggestion! https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:241: const auto& objectStoreIterator = m_oldStoreMetadata.find(objectStore); On 2016/09/26 22:23:40, jsbell wrote: > Can you add a comment about what this case means? Done. Future me thanks you! https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:246: DCHECK(oldStoreMetadata); On 2016/09/26 22:23:40, jsbell wrote: > Can you add a comment about what this case means? Done. Once again, future me thanks you! https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:511: // Remove references to the IDBObjectStore and IDBIndex instances held by On 2016/09/26 22:23:40, jsbell wrote: > This comment is misleading; they types can be GC'd even without this since there > should be no external references and Oilpan understands cycles. We also > shouldn't refer to the project name ("Oilpan") itself in the comments. > > Per comment in the old code, in theory the rest of this method (from here down) > can be deleted now. Gah, sorry for the poor explanation. I was thinking of the case where JavaScript holds on to the IDBTransaction instance for some reason. (Our edge-case tests tend to do that, but I don't think they're representative.) In that case, I think the IDBTransaction will hold onto all the IDBObjectStore instances in its m_objectStoreMap (and, transitively, onto the IDBIndex instances that they have in their "m_indexMap"s), even if those instances are not referenced by JavaScript. Do you think this case is worth handling (and explaining)? If not, I'd be happy to delete code :) https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:91: // implementations need to access the metadata values before the change. On 2016/09/26 22:23:40, jsbell wrote: > "some implementations" sounds odd; does this mean tests? I that the implementations of some of the methods below assume this sequencing. I wanted a consistent sequencing for all these callbacks, so I don't have to remember a lot of details, and it seems to me that invoking the callbacks before changing the metadata requires less mental state than changing the metadata first and passing old information to the callbacks. WDYT? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:122: int64_t oldMaxObjectStoreId() const On 2016/09/26 22:23:40, jsbell wrote: > This is odd data to expose just for this use case, and means the logic is > fragile (e.g. if we switched IDs to be UUIDs instead or something). > > Can this be redone as e.g. a "isNewlyCreated()" method that does the checks > internally, or even just holds a flag? I'm not positive that's a huge > improvement, though. > > Perhaps (per below), each store/index holds its own m_oldMetadata member; if > that's null then the store/index is newly created in this transaction. TBH, I was trying to be clever and avoid a few (cache-trashing) HashMap lookups. I think we can do HashMap lookups everywhere I introduced this trick. I avoided adding versionchange-specific members outside IDBTransaction, because I figured that versionchange transactions should be much less frequent (at most one per site push) than readonly / readwrite transactions (at least one per pageview). I'm open to any of the suggestions here: (1) isNewlyCreated() on IDBObjectStore and IDBIndex (2) take the hit for HashMap lookups (3) m_oldMetadata members How would you like to proceed? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:194: HeapHashMap<Member<IDBObjectStore>, RefPtr<IDBObjectStoreMetadata>> m_oldStoreMetadata; On 2016/09/26 22:23:40, jsbell wrote: > What do you think about storing the data on the stores themselves, e.g. in an > m_oldMetadata member? After having explained my rationale above, I will follow your decision :) https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:215: // Shallow snapshot of the database metadata when the transaction start. On 2016/09/26 22:23:40, jsbell wrote: > nit: wording; should that be "starts" ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pwnall@chromium.org changed reviewers: + cmumford@chromium.org
cmumford: Can you PTAL? jsbell OK-ed the tests, and we're wondering what you think about the implementation changes. We're especially interested in the bit discussed at https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... Thank you!
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:133: void IDBDatabase::setDatabaseMetadata(const IDBDatabaseMetadata& metadata) { I worry that the knowledge of which members to copy, and which to leave as-is exists here in IDBDatabase. If, in the future, a new member is added to IDBDatabaseMetadata, how will that author know to come here. Can this be moved into a new method in IDBDatabaseMetadata? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:45: // these objects form a tree with the hierarch shown below. typo https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:98: public: public not needed - in a struct. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:99: using ObjectStoreMap = HashMap<int64_t, RefPtr<IDBObjectStoreMetadata>>; My preference is to not declare ObjectStoreMap. Just use HashMap below, and use auto in IDBDatabase.cpp. One less level of indirection. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:412: WebIDBDatabase* IDBTransaction::backendDB() const { Why move method within file? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:516: for (auto& it : m_objectStoreMap) { for (auto& objectStore : m_objectStoreMap.values()) { https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:529: for (auto& it : m_oldStoreMetadata) { for (auto& objectStore : m_oldStoreMetadata.keys()) { https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:536: for (auto& it : m_deletedIndexes) { Why not just? for (auto& index : m_deletedIndexes) { https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:541: RefPtr<IDBObjectStoreMetadata> oldMedata = Do you need oldMetadata? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:107: IDBIndex*); // only called when the index's IDBIndex had been created Might read a little cleaner if comment is above declaration. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:231: HeapVector<Member<IDBIndex>> m_deletedIndexes; I wonder if we can/should make an IDBVersionChangeTransaction in some future CL?
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Woohoo! Thank you very much for the review! I have a couple of questions below. Can you please take a look? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:133: void IDBDatabase::setDatabaseMetadata(const IDBDatabaseMetadata& metadata) { On 2016/10/05 21:15:26, cmumford wrote: > I worry that the knowledge of which members to copy, and which to leave as-is > exists here in IDBDatabase. If, in the future, a new member is added to > IDBDatabaseMetadata, how will that author know to come here. Can this be moved > into a new method in IDBDatabaseMetadata? Done. Thank you! I didn't like the assignments, but I couldn't come up with a fix right away. This is a really good idea! https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:45: // these objects form a tree with the hierarch shown below. On 2016/10/05 21:15:27, cmumford wrote: > typo Done. Thank you! https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:98: public: On 2016/10/05 21:15:26, cmumford wrote: > public not needed - in a struct. Done. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:99: using ObjectStoreMap = HashMap<int64_t, RefPtr<IDBObjectStoreMetadata>>; On 2016/10/05 21:15:26, cmumford wrote: > My preference is to not declare ObjectStoreMap. Just use HashMap below, and use > auto in IDBDatabase.cpp. One less level of indirection. Done. Also removed IDBObjectStoreMetadata::IndexMap. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:412: WebIDBDatabase* IDBTransaction::backendDB() const { On 2016/10/05 21:15:27, cmumford wrote: > Why move method within file? I moved it to match the order in the header -- I'm guessing the method became public at some point, which caused it to move in the header, and the move wasn't matched in the .cpp file. If we don't care about the ordering, I can move it back. WDYT? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:516: for (auto& it : m_objectStoreMap) { On 2016/10/05 21:15:27, cmumford wrote: > for (auto& objectStore : m_objectStoreMap.values()) { Done. Cool, thank you for teaching me this! https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:529: for (auto& it : m_oldStoreMetadata) { On 2016/10/05 21:15:27, cmumford wrote: > for (auto& objectStore : m_oldStoreMetadata.keys()) { I need the value too, though -- I'm saving it as oldMetadata two lines below. I think (but don't know for sure) that using HashMap::get() to get the value for each key is more expensive than extracting the keys and values from the iterator. WDYT? https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:536: for (auto& it : m_deletedIndexes) { On 2016/10/05 21:15:27, cmumford wrote: > Why not just? > > for (auto& index : m_deletedIndexes) { Done. Thank you for catching this! https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:541: RefPtr<IDBObjectStoreMetadata> oldMedata = On 2016/10/05 21:15:27, cmumford wrote: > Do you need oldMetadata? Done. Derp, I didn't. I thought we'd have a compiler warning for unused variables, so I wouldn't look dumb? Thanks for catching this! https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:107: IDBIndex*); // only called when the index's IDBIndex had been created On 2016/10/05 21:15:27, cmumford wrote: > Might read a little cleaner if comment is above declaration. Done. Wow, the new formatting really messed this one up :D https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:231: HeapVector<Member<IDBIndex>> m_deletedIndexes; On 2016/10/05 21:15:27, cmumford wrote: > I wonder if we can/should make an IDBVersionChangeTransaction in some future CL? I was thinking about the same thing!! I tried it out while preparing this CL, and I stopped when the diff got too big. I was also scared of perf regressions because of BTB trashing introduced by virtual methods. I filed http://crbug.com/653295 to track this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:122: int64_t oldMaxObjectStoreId() const On 2016/09/27 00:12:19, pwnall wrote: > On 2016/09/26 22:23:40, jsbell wrote: > > This is odd data to expose just for this use case, and means the logic is > > fragile (e.g. if we switched IDs to be UUIDs instead or something). > > > > Can this be redone as e.g. a "isNewlyCreated()" method that does the checks > > internally, or even just holds a flag? I'm not positive that's a huge > > improvement, though. > > > > Perhaps (per below), each store/index holds its own m_oldMetadata member; if > > that's null then the store/index is newly created in this transaction. > > TBH, I was trying to be clever and avoid a few (cache-trashing) HashMap lookups. > I think we can do HashMap lookups everywhere I introduced this trick. > > I avoided adding versionchange-specific members outside IDBTransaction, because > I figured that versionchange transactions should be much less frequent (at most > one per site push) than readonly / readwrite transactions (at least one per > pageview). > > I'm open to any of the suggestions here: > (1) isNewlyCreated() on IDBObjectStore and IDBIndex > (2) take the hit for HashMap lookups > (3) m_oldMetadata members > > How would you like to proceed? #3 if possible (assuming I'm correct that m_oldMetadata is the right signal), #1 if not. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:412: WebIDBDatabase* IDBTransaction::backendDB() const { On 2016/10/05 23:15:45, pwnall wrote: > On 2016/10/05 21:15:27, cmumford wrote: > > Why move method within file? > > I moved it to match the order in the header -- I'm guessing the method became > public at some point, which caused it to move in the header, and the move wasn't > matched in the .cpp file. > > If we don't care about the ordering, I can move it back. WDYT? I'd leave it alone for this CL since it's untouched, do cleanup in a separate CL just to minimize the diffs. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:529: for (auto& it : m_oldStoreMetadata) { On 2016/10/05 23:15:45, pwnall wrote: > On 2016/10/05 21:15:27, cmumford wrote: > > for (auto& objectStore : m_oldStoreMetadata.keys()) { > > I need the value too, though -- I'm saving it as oldMetadata two lines below. I > think (but don't know for sure) that using HashMap::get() to get the value for > each key is more expensive than extracting the keys and values from the > iterator. > > WDYT? Seems fine to me. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:541: RefPtr<IDBObjectStoreMetadata> oldMedata = On 2016/10/05 23:15:45, pwnall wrote: > On 2016/10/05 21:15:27, cmumford wrote: > > Do you need oldMetadata? > > Done. > Derp, I didn't. > > I thought we'd have a compiler warning for unused variables, so I wouldn't look > dumb? > Thanks for catching this! cpplint.py may be handy - not run by default; not sure if it would have caught this. Also, need to ignore several things due to blink style/legacy code. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:231: HeapVector<Member<IDBIndex>> m_deletedIndexes; On 2016/10/05 23:15:45, pwnall wrote: > On 2016/10/05 21:15:27, cmumford wrote: > > I wonder if we can/should make an IDBVersionChangeTransaction in some future > CL? > > I was thinking about the same thing!! > > I tried it out while preparing this CL, and I stopped when the diff got too big. > I was also scared of perf regressions because of BTB trashing introduced by > virtual methods. > > I filed http://crbug.com/653295 to track this. Agreed, separate CL. (Perf is almost certainly not an issue; the serialize/deserialize/IPC overhead from most IDB operations is orders of magnitude more expensive than the impact of virtual methods here is likely to be. It does matter for things like DOM node manipulation which is synchronous and has cases where 100k operations are not uncommon in big documents.) https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.h:70: // metadata. Can you clarify in the comments why we need both of these? E.g. "Used when opening the connection" / "Used when aborting an upgrade transaction" https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBIndex.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBIndex.h:92: DCHECK(m_transaction->isVersionChange()) This should probably move into .cpp file. (Otherwise I think every compilation unit that includes this header will end up with a copy of the string, albeit only in debug builds.) https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:45: // these objects form a tree with the hierarch shown below. typo: hierarchy https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:109: void copyDatabaseMetadataFrom(const IDBDatabaseMetadata&); Simplify to just 'copyFrom' ? https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (left): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:118: backendDB()->renameObjectStore(m_transaction->id(), id(), name); IDBIndex::setName calls WebIDBDatabase::renameIndex directly, but IDBObjectStore::setName relies on IDBDatabase to call WebIDBDatabase::renameObjectStore - can we make these consistent? https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:231: // No IDBObjectStore instance was created for the deleted store in this Is there an observable case where no IDBObjectStore instance was created, yet the metadata needs reverting? https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:543: m_database->setDatabaseMetadata(m_oldDatabaseMetadata); Maybe comment here that this is used (rather than setMetadata) because the set of stores that should be present are reverted above? https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:555: // this transaction, so OilPan can garbage-collect the instances that aren't spelling: OilPan -> Oilpan
https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:412: WebIDBDatabase* IDBTransaction::backendDB() const { What Josh said, plus moving it also makes it less likely that "git blame" will tell you who added the function and why. https://codereview.chromium.org/2314933005/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:529: for (auto& it : m_oldStoreMetadata) { Yeah, if you need both key and value then absolutely stick with the iterator and don't call get().
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #14 (id:580001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell: PTAL? https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2314933005/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:122: int64_t oldMaxObjectStoreId() const On 2016/10/06 20:01:31, jsbell wrote: > On 2016/09/27 00:12:19, pwnall wrote: > > On 2016/09/26 22:23:40, jsbell wrote: > > > This is odd data to expose just for this use case, and means the logic is > > > fragile (e.g. if we switched IDs to be UUIDs instead or something). > > > > > > Can this be redone as e.g. a "isNewlyCreated()" method that does the checks > > > internally, or even just holds a flag? I'm not positive that's a huge > > > improvement, though. > > > > > > Perhaps (per below), each store/index holds its own m_oldMetadata member; if > > > that's null then the store/index is newly created in this transaction. > > > > TBH, I was trying to be clever and avoid a few (cache-trashing) HashMap > lookups. > > I think we can do HashMap lookups everywhere I introduced this trick. > > > > I avoided adding versionchange-specific members outside IDBTransaction, > because > > I figured that versionchange transactions should be much less frequent (at > most > > one per site push) than readonly / readwrite transactions (at least one per > > pageview). > > > > I'm open to any of the suggestions here: > > (1) isNewlyCreated() on IDBObjectStore and IDBIndex > > (2) take the hit for HashMap lookups > > (3) m_oldMetadata members > > > > How would you like to proceed? > > #3 if possible (assuming I'm correct that m_oldMetadata is the right signal), #1 > if not. Done. We decided on #1 in an offline discussion. The code is a lot more readable now. Thank you very much! https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.h:70: // metadata. On 2016/10/06 20:01:31, jsbell wrote: > Can you clarify in the comments why we need both of these? E.g. "Used when > opening the connection" / "Used when aborting an upgrade transaction" Done. https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBIndex.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBIndex.h:92: DCHECK(m_transaction->isVersionChange()) On 2016/10/06 20:01:31, jsbell wrote: > This should probably move into .cpp file. (Otherwise I think every compilation > unit that includes this header will end up with a copy of the string, albeit > only in debug builds.) Isn't that an acceptable tradeoff in return for smaller code size on release? I'm reasonably sure that the setter (address calculation + load, a single mov on Intel) will be smaller than a method call (push + jump). https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:45: // these objects form a tree with the hierarch shown below. On 2016/10/06 20:01:31, jsbell wrote: > typo: hierarchy Done. Gah... I swore I fixed this when addressing cmumford's feedback. Sorry, and thank you so much for catching my mistake! https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h:109: void copyDatabaseMetadataFrom(const IDBDatabaseMetadata&); On 2016/10/06 20:01:31, jsbell wrote: > Simplify to just 'copyFrom' ? > Done. FWIW, I think that copyFrom is a bit of a lie, because we don't copy store metadata, but the name I had before isn't any better. Thank you! https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (left): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:118: backendDB()->renameObjectStore(m_transaction->id(), id(), name); On 2016/10/06 20:01:31, jsbell wrote: > IDBIndex::setName calls WebIDBDatabase::renameIndex directly, but > IDBObjectStore::setName relies on IDBDatabase to call > WebIDBDatabase::renameObjectStore - can we make these consistent? Done. This is a great suggestion! Thank you! https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:231: // No IDBObjectStore instance was created for the deleted store in this On 2016/10/06 20:01:31, jsbell wrote: > Is there an observable case where no IDBObjectStore instance was created, yet > the metadata needs reverting? Calling IDBDatabase.deleteObjectStore will remove the store from the database's metadata. We need to be able to put the store metadata back if the transaction is aborted, so objectStoreNames returns correct values. I expanded this comment and added a test that passes for both us and Firefox. https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:543: m_database->setDatabaseMetadata(m_oldDatabaseMetadata); On 2016/10/06 20:01:31, jsbell wrote: > Maybe comment here that this is used (rather than setMetadata) because the set > of stores that should be present are reverted above? Done. https://codereview.chromium.org/2314933005/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:555: // this transaction, so OilPan can garbage-collect the instances that aren't On 2016/10/06 20:01:31, jsbell wrote: > spelling: OilPan -> Oilpan > Done.
lgtm
On 2016/10/06 22:31:51, jsbell wrote: > lgtm Thank you for the quick turnaround!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/2314933005/#ps600001 (title: "Addressed feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Align IndexedDB metadata rollback on transaction abort to spec. When an IndexedDB versionchange transaction is aborted, Blink's IndexedDB metadata is not entirely reverted, causing our behavior to deviate from the IndexedDB specification in minor, subtle ways. This change aligns our behavior with the spec and with Firefox's implementation. The change also (slightly) reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ========== to ========== Align IndexedDB metadata rollback on transaction abort to spec. When an IndexedDB versionchange transaction is aborted, Blink's IndexedDB metadata is not entirely reverted, causing our behavior to deviate from the IndexedDB specification in minor, subtle ways. This change aligns our behavior with the spec and with Firefox's implementation. The change also (slightly) reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Align IndexedDB metadata rollback on transaction abort to spec. When an IndexedDB versionchange transaction is aborted, Blink's IndexedDB metadata is not entirely reverted, causing our behavior to deviate from the IndexedDB specification in minor, subtle ways. This change aligns our behavior with the spec and with Firefox's implementation. The change also (slightly) reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 ========== to ========== Align IndexedDB metadata rollback on transaction abort to spec. When an IndexedDB versionchange transaction is aborted, Blink's IndexedDB metadata is not entirely reverted, causing our behavior to deviate from the IndexedDB specification in minor, subtle ways. This change aligns our behavior with the spec and with Firefox's implementation. The change also (slightly) reduces IndexedDB memory usage in two ways. (1) Object store and index metadata is now shared between an IDBDatabase and its associated IDBObjectStore and IDBIndex instances, instead of being copied. (2) versionchange transactions only back up the metadata for object stores that are accessed by JavaScript, instead of creating a backup for the entire database metadata. BUG=645018,457447 Committed: https://crrev.com/3572fef3913166c5da29dca38c04bf6f6683560d Cr-Commit-Position: refs/heads/master@{#423742} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3572fef3913166c5da29dca38c04bf6f6683560d Cr-Commit-Position: refs/heads/master@{#423742} |
