Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1071553002:
Optimizes the operation of DeleteDatabase when no backing store exists. (Closed)
|
Created:
5 years, 8 months ago by payal.pandey Modified:
5 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, dgrogan, jam, jsbell+idb_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimizes the operation of DeleteDatabase when no backing store exists.
When deleting the non existing database in a origin with no existing
backing store, then in DeleteDatabse(), early exiting before OpenBackingStore,
so that it wont results in multiple transaction as earlier.
BUG=460236
Committed: https://crrev.com/a4c618a142515773709688bfe3c5753e589afd43
Cr-Commit-Position: refs/heads/master@{#326473}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Optimizes the operation of DeleteDatabase when no backing store exists. #
Total comments: 1
Patch Set 3 : Optimizes the operation of DeleteDatabase when no backing store exists. #
Total comments: 7
Patch Set 4 : Optimizes the operation of DeleteDatabase when no backing store exists. #
Total comments: 6
Patch Set 5 : Incorporated review comments for browser tests. #
Messages
Total messages: 37 (8 generated)
payal.pandey@samsung.com changed reviewers: + jsbell@chromium.org
please have a look and review..
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_factory_impl.cc:240: IndexedDBBackingStoreMap::iterator it2 = backing_store_map_.find(origin_url); Just call IsBackingStoreOpen(origin_url) https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_factory_impl.cc:241: if(it2 == backing_store_map_.end()) spaces, and { are incorrect. Run "git cl format" to have your formatting fixed. https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_factory_impl.cc:245: } Why not have this test at the very beginning of this function?
BTW I don't believe this CL satisfies the issue requirements. backing_store_map_ is only a map of open (not existing) backing stores. Consider the case where a page created a database a week ago, and now just calls window.indexedDB.deleteDatabase(...). I'm not sure that the backing store will ever get deleted with this change.
Modified the early exit after OpenBackingStore and before Database creation in DeleteDatabase() And instead of backing_store_map_ (which is map of opened backing stores) used backing_store->GetDatabaseNames, which is vector of existing database names in that origin. Thanks for sharing the information Please review the patch.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
This change breaks some content browser tests (content_browsertests) and Blink layout tests so this change does not LGTM. Also, still not correctly formatted. See my earlier review comment. https://codereview.chromium.org/1071553002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:274: if(findItr == names.end()) You aren't using findIter so just use: if (std::find(names.begin(), names.end(), name) == names.end())
Modified the CL as per review comment: > This change breaks some content browser tests (content_browsertests) and Blink > layout tests so this change does not LGTM. --Fixed for content_browsertests (i.e DiskFullOnCommit) --Fixed for Blink layout tests > Also, still not correctly formatted. See my earlier review comment. --Run (git cl presubmit) which already includes (git cl format). --Removed findIter code.
https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:265: "indexedDB.webkitGetDatabaseNames."); This should be "indexedDB.deleteDatabase" https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:272: if(std::find(names.begin(), names.end(), name) == names.end()) #include "base/stl_util.h" and use: base::ContainsKey(names, name) https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:272: if(std::find(names.begin(), names.end(), name) == names.end()) Space between if and ( please use `git cl format` before uploading https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:273: { { should be on same line as if please use `git cl format` before uploading https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:274: int64 version = 0; Make this const https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:277: ReleaseBackingStore(origin_url, false ); No space before ) please use `git cl format` before uploading. https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed... content/browser/indexed_db/indexed_db_factory_impl.cc:277: ReleaseBackingStore(origin_url, false ); Please document the use of false here (e.g. /* immediate */ false)
Incorporated review comments.
On 2015/04/15 09:41:49, payal.pandey wrote: > Incorporated review comments. Please review the patch..
The CQ bit was checked by jsbell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071553002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I don't believe this change will fix the bug, and is also harmful. There's only one backing store per origin, but that one backing store can contain multiple IDB databases. This CL doesn't early out when there is no backing store, but instead does so when it doesn't contain the requested database - but it may contain others - which may in fact be open! Let me know if I'm missing something obvious, but this still does not LGTM. https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_factory_impl.cc:264: "Internal error opening backing store for " Error string incorrect (copied from above).
Discussed with another dev here (jsbell), and this CL may be OK. We're going to take a closer look at it later on and give you a final verdict. At present no changes requested.
On 2015/04/16 20:10:58, cmumford wrote: > Discussed with another dev here (jsbell), and this CL may be OK. We're going to > take a closer look at it later on and give you a final verdict. At present no > changes requested.
when can I expect for its final response
On 2015/04/20 at 06:55:29, payal.pandey wrote: > when can I expect for its final response LGTM - I think jsbell wanted to give it one final look, but I'm OK with it.
Yes, I'm sorry for the delay. The code looks correct in context, given that it behaves the same way as the GetDatabaseNames path. Given that GetDatabaseNames is nonstandard and not widely used, though, I wanted to double check the relationship between the factory and backing store lifetimes. I'm at a conference today but I'll try to get through this tomorrow. Thanks for your patience! BTW, there is the opportunity for another optimization if the backing store doesn't exist, but that should be another CL.
Yes, I'm sorry for the delay. The code looks correct in context, given that it behaves the same way as the GetDatabaseNames path. Given that GetDatabaseNames is nonstandard and not widely used, though, I wanted to double check the relationship between the factory and backing store lifetimes. I'm at a conference today but I'll try to get through this tomorrow. Thanks for your patience! BTW, there is the opportunity for another optimization if the backing store doesn't exist, but that should be another CL.
On 2015/04/20 16:27:59, jsbell wrote: > Yes, I'm sorry for the delay. The code looks correct in context, given that it > behaves the same way as the GetDatabaseNames path. > > Given that GetDatabaseNames is nonstandard and not widely used, though, I wanted > to double check the relationship between the factory and backing store > lifetimes. I'm at a conference today but I'll try to get through this tomorrow. > > Thanks for your patience! > > BTW, there is the opportunity for another optimization if the backing store > doesn't exist, but that should be another CL. Sounds great.. for next optimization opportunity. I would like to work on that too. If possible please raise new issue & share the ID for me to work on it.
I wondered, if you please look in this CL, today, whether it can be closed.
Thanks for your patience! lgtm if the test comments are fixed. https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_browsertest.cc:495: // * Then deletes the database: This part (lines 495 through 502) no longer happens. So delete this part of the comment... https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_browsertest.cc:504: // #5: IndexedDBFactoryImpl::Open And renumber this to #3 https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_browsertest.cc:507: // #6: IndexedDBTransaction::Commit - initial "versionchange" transaction And renumber this to #4 https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_browsertest.cc:508: // * Once the connection is opened, the test runs: And this becomes #5, matching the instance_num change (yay!) https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_factory_impl.cc:264: "Internal error opening backing store for " On 2015/04/16 17:22:47, cmumford wrote: > Error string incorrect (copied from above). As cmumford noted, his comment was a misunderstanding. This error message here is correct.
On 2015/04/21 08:33:52, payal.pandey wrote:
> Sounds great.. for next optimization opportunity.
> I would like to work on that too.
> If possible please raise new issue & share the ID for me to work on it.
It's covered in issue 460236 as well:
#1: IndexedDBBackingStore::OpenBackingStore
=> IndexedDBBackingStore::SetUpMetadata
^^^ We could early-exit if the backing store does not already exist.
Before coding, you should check with cmumford (e.g. discuss in the bug) to agree
on a heuristic for "does not already exist". For example, we could introduce a
static 'DoesBackingStoreExist' method on IndexedDBBackingStore to test, and
simply see if the directory is present. We'd want to ensure it functions
correctly for incognito and normal contexts.
The CQ bit was checked by payal.pandey@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/1071553002/#ps120001 (title: "Incorporated review comments for browser tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071553002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a4c618a142515773709688bfe3c5753e589afd43 Cr-Commit-Position: refs/heads/master@{#326473}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1106703003/ by jyasskin@chromium.org. The reason for reverting is: This appears to have made IndexedDBBrowserTest.DiskFullOnCommit flaky, starting at http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20... [ RUN ] IndexedDBBrowserTest.DiskFullOnCommit [1028:1768:0423/085845:4229748:INFO:mock_browsertest_indexed_db_class_factory.cc(258)] FailOperation: class=2, method=2, instanceNum=5, callNum=1 [1028:1768:0423/085845:4229873:INFO:indexed_db_browsertest.cc(77)] Navigating to URL and blocking. [3872:2724:0423/085859:4243383:ERROR:renderer_main.cc(200)] Running without renderer sandbox [1028:1096:0423/085958:4302491:ERROR:indexed_db_backing_store.cc(4197)] IndexedDB Write Error: TRANSACTION_COMMIT_METHOD [1028:1768:0423/090005:4309621:INFO:indexed_db_browsertest.cc(79)] Navigation done. c:\b\build\slave\drm-cr\build\src\content\browser\indexed_db\indexed_db_browsertest.cc(88): error: Failed Failed: Starting...<span>FAILED: unexpectedErrorCallback<br></span> [ FAILED ] IndexedDBBrowserTest.DiskFullOnCommit, where TypeParam = and GetParam() = (130664 ms) .
Message was sent while issue was closed.
Attempting reland over at https://codereview.chromium.org/1216573008/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
