Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
477973002:
Add Keys() function to ServiceWorkerCache. (Closed)
|
Created:
6 years, 4 months ago by jkarlin Modified:
6 years, 3 months ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cache_patch Project:
chromium Visibility:
Public. |
DescriptionAdd Keys() function to ServiceWorkerCache.
Other changes:
* Introduced a ReadHeaders helper function which reads headers from a cache entry into a protobuf. This is used by Match() and Keys().
BUG=392621
Committed: https://crrev.com/cd6ec97606ab4b18c83199e873b28e6ef7846718
Cr-Commit-Position: refs/heads/master@{#292945}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Now what the spec expects #Patch Set 4 : nits #Patch Set 5 : Big Rebase #Patch Set 6 : Nits after rebase #Patch Set 7 : Rebase #Patch Set 8 : Nits #Patch Set 9 : Rebase #
Total comments: 22
Patch Set 10 : Rebase #Patch Set 11 : Addresses comments from PS9 #
Total comments: 8
Patch Set 12 : Address comments from PS11 and enable SimpleCache everywhere #
Total comments: 4
Patch Set 13 : Remove accidental change to entry unittest #
Messages
Total messages: 23 (2 generated)
jkarlin@chromium.org changed reviewers: + falken@chromium.org, michaeln@chromium.org
michaeln@chromium.org: Please review changes in all falken@chromium.org: Please review changes in all
took me a while to read thru https://codereview.chromium.org/477973002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:51: entries->at(i)->Close(); delete entries too? https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:558: scoped_ptr<void*> iter, might be nice to distinguish the void* iter from the Entries::iterator iter via naming thruout https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:566: new ServiceWorkerCache::Requests()); ah ha, here's where the collection to hold the result of the keys method is cons'd up! you might want to refer to this as out_keys to make that connection more clear? https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:567: return KeysProcessNextEntry( its odd to see return <something> in a function with a void return value? i'd vote to have return; on the next line instead https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:590: int rvv = looks like you could just use the existing parameter/variable 'rv' here, i was wondering if rvv was a typo https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:591: cache->backend()->OpenNextEntry(iter_ptr, entry_ptr, open_entry_callback); i was wondering what caused backend() to be added to the public interface. seems like this class should more thouroughly encapsulate the backend. if these anon function where all static instance methods, backend* would not have to be exposed. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:602: return callback.Run(ServiceWorkerCache::ErrorTypeOK, requests.Pass()); This is the end of the line in the expected case, is that right? nit: its impressive that the bind mechanism can express ownership and be used to xfer ownership down the line, but it doesn't always make for the most grokkable code. In Keys(), these ptrs to ptrs are created and kept alive thru a very long series of completion callbacks. 830 scoped_ptr<void*> iter(new void*(NULL)); 831 scoped_ptr<disk_cache::Entry*> entry(new disk_cache::Entry*); 839 ScopedEntriesPtr entries(new Entries()); Additional values are added to the long lived 'state' thats kept alive at key points in the processing. 564 Entries::iterator iter = entries->begin(); // not to be confused with scoped_ptr<void*> iter 565 scoped_ptr<ServiceWorkerCache::Requests> out_requests( All the ptr-to-ptr indirection obscures whats going on, ServiceWorkerCache::Requests is really just a std::vector<T>, and understanding where objects are created and finally deleted is like hunting for a ghost. Personal opinion but i think this would be easier to follow and maintain if there was one struct to bundle up the state needed to process a Keys() method call and that one thing was new'd at the beginning and Passed down the line. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:621: entry->Doom(); Interesting, a subsequent call to Keys() will yeild a different result since the iteration will continue at this point to the next item. Would it make sense to Doom the botched entry here, but to also continue the iteration so subsequent calls would be expected to get the same results. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:764: return callback.Run(ErrorTypeStorage); why? https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:836: // Load up all of the entries into a vector and then read the request data It might help to expand on this comment to better explain the sequence of events, its not so straightforward from a reading of the code. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:838: // entries are altered (such as reading header data) while enumerating. how does reading header data alter it?
Patchset #11 (id:200001) has been deleted
https://codereview.chromium.org/477973002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:51: entries->at(i)->Close(); On 2014/08/27 23:45:20, michaeln wrote: > delete entries too? Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:558: scoped_ptr<void*> iter, On 2014/08/27 23:45:20, michaeln wrote: > might be nice to distinguish the void* iter from the Entries::iterator iter via > naming thruout Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:566: new ServiceWorkerCache::Requests()); On 2014/08/27 23:45:20, michaeln wrote: > ah ha, here's where the collection to hold the result of the keys method is > cons'd up! you might want to refer to this as out_keys to make that connection > more clear? Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:567: return KeysProcessNextEntry( On 2014/08/27 23:45:20, michaeln wrote: > its odd to see return <something> in a function with a void return value? i'd > vote to have return; on the next line instead Done for the whole file. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:590: int rvv = On 2014/08/27 23:45:20, michaeln wrote: > looks like you could just use the existing parameter/variable 'rv' here, i was > wondering if rvv was a typo Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:591: cache->backend()->OpenNextEntry(iter_ptr, entry_ptr, open_entry_callback); On 2014/08/27 23:45:20, michaeln wrote: > i was wondering what caused backend() to be added to the public interface. seems > like this class should more thouroughly encapsulate the backend. if these anon > function where all static instance methods, backend* would not have to be > exposed. You're right, better to hide backend. I'm not gaining any encapsulation with unnamed functions if I have to expose private variables to do it. I need to do the same for a couple other functions later. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:602: return callback.Run(ServiceWorkerCache::ErrorTypeOK, requests.Pass()); On 2014/08/27 23:45:20, michaeln wrote: > This is the end of the line in the expected case, is that right? > > nit: its impressive that the bind mechanism can express ownership and be used to > xfer ownership down the line, but it doesn't always make for the most grokkable > code. > > In Keys(), these ptrs to ptrs are created and kept alive thru a very long series > of completion callbacks. > 830 scoped_ptr<void*> iter(new void*(NULL)); > 831 scoped_ptr<disk_cache::Entry*> entry(new disk_cache::Entry*); > 839 ScopedEntriesPtr entries(new Entries()); > Additional values are added to the long lived 'state' thats kept alive at key > points in the processing. > 564 Entries::iterator iter = entries->begin(); // not to be confused with > scoped_ptr<void*> iter > 565 scoped_ptr<ServiceWorkerCache::Requests> out_requests( > > All the ptr-to-ptr indirection obscures whats going on, > ServiceWorkerCache::Requests is really just a std::vector<T>, and understanding > where objects are created and finally deleted is like hunting for a ghost. > Personal opinion but i think this would be easier to follow and maintain if > there was one struct to bundle up the state needed to process a Keys() method > call and that one thing was new'd at the beginning and Passed down the line. Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:621: entry->Doom(); On 2014/08/27 23:45:20, michaeln wrote: > Interesting, a subsequent call to Keys() will yeild a different result since the > iteration will continue at this point to the next item. Would it make sense to > Doom the botched entry here, but to also continue the iteration so subsequent > calls would be expected to get the same results. Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:764: return callback.Run(ErrorTypeStorage); On 2014/08/27 23:45:20, michaeln wrote: > why? Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:836: // Load up all of the entries into a vector and then read the request data On 2014/08/27 23:45:20, michaeln wrote: > It might help to expand on this comment to better explain the sequence of > events, its not so straightforward from a reading of the code. Done. https://codereview.chromium.org/477973002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:838: // entries are altered (such as reading header data) while enumerating. On 2014/08/27 23:45:20, michaeln wrote: > how does reading header data alter it? It changes the "last_used" field which seems buggy but the comments clearly warn that this might happen. I've clarified the comment.
i have one question about the reading data breaks enumeration behavior... https://codereview.chromium.org/477973002/diff/220001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:603: ~KeysContext() { if (enumerated_entry) enumerated_entry->Close(); ? https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:774: // forever if you read data from a cache entry while enumerating. Is this going to be a problem if/when data retrieval (Match) happens while a Keys() call is still in the first phase? https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:810: if (rv == net::ERR_FAILED) { DCHECK(!enumerated_entry) ? https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:826: keys_context->entries.push_back(keys_context->enumerated_entry); keys_contenxt->enumerated_entry = NULL; ?
https://codereview.chromium.org/477973002/diff/220001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:774: // forever if you read data from a cache entry while enumerating. On 2014/08/28 23:49:39, michaeln wrote: > Is this going to be a problem if/when data retrieval (Match) happens while a > Keys() call is still in the first phase? What a fantastic question. I'm going to look into this today. A leveldb solution is looking better every day...
On 2014/08/29 11:09:38, jkarlin wrote: > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > File content/browser/service_worker/service_worker_cache.cc (right): > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > content/browser/service_worker/service_worker_cache.cc:774: // forever if you > read data from a cache entry while enumerating. > On 2014/08/28 23:49:39, michaeln wrote: > > Is this going to be a problem if/when data retrieval (Match) happens while a > > Keys() call is still in the first phase? > > What a fantastic question. I'm going to look into this today. A leveldb > solution is looking better every day... As hoped, SimpleCache does not have this bad behavior, it's an issue for the block cache. I'll address this by changing over completely to SimpleCache. This means I'll need to disable a couple of unit tests in Windows until we fix SimpleCache in windows.
On 2014/08/29 13:01:25, jkarlin wrote: > On 2014/08/29 11:09:38, jkarlin wrote: > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > File content/browser/service_worker/service_worker_cache.cc (right): > > > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > content/browser/service_worker/service_worker_cache.cc:774: // forever if you > > read data from a cache entry while enumerating. > > On 2014/08/28 23:49:39, michaeln wrote: > > > Is this going to be a problem if/when data retrieval (Match) happens while a > > > Keys() call is still in the first phase? > > > > What a fantastic question. I'm going to look into this today. A leveldb > > solution is looking better every day... > > As hoped, SimpleCache does not have this bad behavior, it's an issue for the > block cache. I'll address this by changing over completely to SimpleCache. > This means I'll need to disable a couple of unit tests in Windows until we fix > SimpleCache in windows. I've talked Gavin into going for a LevelDB + files for blob backend. Hang on tight. Removing reviewers as this won't be needed any longer.
On 2014/08/29 13:01:25, jkarlin wrote: > On 2014/08/29 11:09:38, jkarlin wrote: > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > File content/browser/service_worker/service_worker_cache.cc (right): > > > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > content/browser/service_worker/service_worker_cache.cc:774: // forever if you > > read data from a cache entry while enumerating. > > On 2014/08/28 23:49:39, michaeln wrote: > > > Is this going to be a problem if/when data retrieval (Match) happens while a > > > Keys() call is still in the first phase? > > > > What a fantastic question. I'm going to look into this today. A leveldb > > solution is looking better every day... > > As hoped, SimpleCache does not have this bad behavior, it's an issue for the > block cache. I'll address this by changing over completely to SimpleCache. > This means I'll need to disable a couple of unit tests in Windows until we fix > SimpleCache in windows. I've talked Gavin into going for a LevelDB + files for blob backend. Hang on tight. Removing reviewers as this won't be needed any longer.
On 2014/08/29 13:20:06, jkarlin wrote: > On 2014/08/29 13:01:25, jkarlin wrote: > > On 2014/08/29 11:09:38, jkarlin wrote: > > > > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > > File content/browser/service_worker/service_worker_cache.cc (right): > > > > > > > > > https://codereview.chromium.org/477973002/diff/220001/content/browser/service... > > > content/browser/service_worker/service_worker_cache.cc:774: // forever if > you > > > read data from a cache entry while enumerating. > > > On 2014/08/28 23:49:39, michaeln wrote: > > > > Is this going to be a problem if/when data retrieval (Match) happens while > a > > > > Keys() call is still in the first phase? > > > > > > What a fantastic question. I'm going to look into this today. A leveldb > > > solution is looking better every day... > > > > As hoped, SimpleCache does not have this bad behavior, it's an issue for the > > block cache. I'll address this by changing over completely to SimpleCache. > > This means I'll need to disable a couple of unit tests in Windows until we fix > > SimpleCache in windows. > > I've talked Gavin into going for a LevelDB + files for blob backend. Hang on > tight. Removing reviewers as this won't be needed any longer. Given our 2 week deadline for default on, it's probably better to continue with the SimpleCache version and transition to LevelDB after. So I'll switch to SimpleCache everywhere (in a different CL) and fix up Keys() so that it iterates properly.
jkarlin@chromium.org changed reviewers: + gavinp@chromium.org - falken@chromium.org
+gavinp for discussion Okay so here is the deal: SimpleCache is fine with enumeration. BlockFileCache is not fine with enumeration. MemoryCache is fine with enumeration because it's synchronous. So we need to use SimpleCache everywhere. SimpleCache is a bit broken on Windows (note that I had to comment out a couple of tests). Gavin can go further into how and why. But for an initial release Gavin suggests that it should be OK. https://codereview.chromium.org/477973002/diff/220001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:603: ~KeysContext() { On 2014/08/28 23:49:39, michaeln wrote: > if (enumerated_entry) enumerated_entry->Close(); ? Done. https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:810: if (rv == net::ERR_FAILED) { On 2014/08/28 23:49:39, michaeln wrote: > DCHECK(!enumerated_entry) ? Done. https://codereview.chromium.org/477973002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:826: keys_context->entries.push_back(keys_context->enumerated_entry); On 2014/08/28 23:49:39, michaeln wrote: > keys_contenxt->enumerated_entry = NULL; ? Done.
lgtm https://codereview.chromium.org/477973002/diff/260001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/260001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:813: DCHECK(!keys_context->enumerated_entry); btw, ty for switching to having a KeysContext struct https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... net/disk_cache/entry_unittest.cc:2452: TEST_F(DiskCacheEntryTest, SimpleCacheReadOriteDestroyBuffer) { typo?
oh, the cl description is a little stale now
https://codereview.chromium.org/477973002/diff/260001/content/browser/service... File content/browser/service_worker/service_worker_cache.cc (right): https://codereview.chromium.org/477973002/diff/260001/content/browser/service... content/browser/service_worker/service_worker_cache.cc:813: DCHECK(!keys_context->enumerated_entry); On 2014/08/29 20:08:26, michaeln wrote: > btw, ty for switching to having a KeysContext struct It's a bit improvement. Thanks for suggesting it. https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... net/disk_cache/entry_unittest.cc:2452: TEST_F(DiskCacheEntryTest, SimpleCacheReadOriteDestroyBuffer) { On 2014/08/29 20:08:26, michaeln wrote: > typo? Done.
On 2014/09/02 11:31:38, jkarlin wrote: > https://codereview.chromium.org/477973002/diff/260001/content/browser/service... > File content/browser/service_worker/service_worker_cache.cc (right): > > https://codereview.chromium.org/477973002/diff/260001/content/browser/service... > content/browser/service_worker/service_worker_cache.cc:813: > DCHECK(!keys_context->enumerated_entry); > On 2014/08/29 20:08:26, michaeln wrote: > > btw, ty for switching to having a KeysContext struct > > It's a bit improvement. Thanks for suggesting it. s/bit/big/ > > https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... > File net/disk_cache/entry_unittest.cc (right): > > https://codereview.chromium.org/477973002/diff/260001/net/disk_cache/entry_un... > net/disk_cache/entry_unittest.cc:2452: TEST_F(DiskCacheEntryTest, > SimpleCacheReadOriteDestroyBuffer) { > On 2014/08/29 20:08:26, michaeln wrote: > > typo? > > Done.
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@chromium.org/477973002/280001
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as 02812f0a3edcacca1f7e05ea0a47cb3d8c6025e1
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:280001) has been created in https://codereview.chromium.org/534683002/ by scottmg@chromium.org. The reason for reverting is: Failing on content_unittests EmptyKeys_1 TwoKeys_1 TwoKeysThenOne_1 on Linux ASAN LSAN.
Message was sent while issue was closed.
On 2014/09/02 18:18:13, scottmg wrote: > A revert of this CL (patchset #13 id:280001) has been created in > https://codereview.chromium.org/534683002/ by mailto:scottmg@chromium.org. > > The reason for reverting is: Failing on content_unittests > > EmptyKeys_1 > TwoKeys_1 > TwoKeysThenOne_1 > > on Linux ASAN LSAN. Failures visible at: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... The iterator object (KeysContext::backend_iterator_) isn't being cleaned up. From the headers, if the iterator is not driven to the end then EndEnumeration(iter) needs to be called - presumably drop cache->backend_->EndEnumerat(&keys_context->backend_iterator_) into the early exit in ServiceWorkerCache::KeysDidOpenNextEntry?
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/cd6ec97606ab4b18c83199e873b28e6ef7846718 Cr-Commit-Position: refs/heads/master@{#292945} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
