Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14130015:
Support overlapping operations on the SimpleEntryImpl. (Closed)
|
Created:
7 years, 8 months ago by felipeg Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport overlapping operations on the SimpleEntryImpl.
Using a std::queue<Closure>, it serializes the operations and runs each operation in the correct order.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194908
Patch Set 1 #Patch Set 2 : fix a bug in GetDataSize() #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : add unittests #
Total comments: 5
Patch Set 7 : #
Total comments: 12
Patch Set 8 : #
Total comments: 4
Patch Set 9 : Merging with Gavin scoped_refptr CL #Patch Set 10 : Egor comments #
Total comments: 12
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : rebase with gavin CL #Patch Set 14 : rebase again #
Total comments: 1
Messages
Total messages: 22 (0 generated)
Guys Check this out this crazy idea might work it works in fact, it worked in my workstation , and I could see things running in parallel without problems
I like this approach, and I think it's more important to land this than to get truly overlapping Reads. Just think of all the unit test coverage this unlocks. For now I think there are no truly "read only" operations, since SimpleSynchronousEntry mutates itself on all operations. Might as well just make this CL implement the queue only. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:89: void SimpleEntryImpl::Doom() { I think doom is one of the only operations that already can be run completely concurrently with anything else (including other Dooms). https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:104: // around the code, to see if we dont call read in a doomed entry. I believe it's correct and valid to Read/Write in a doomed entry.
This is great. Very simple. Polished up, I think this is a high priority to land. We should turn on as many entry_unittests as possible at the same time as we put this in. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:222: index, offset, buf, This is really dangerous, calling with the naked buf. The problem is that the caller can drop their ref to buf, and then buf is deleted under us. The existing calls used make_scoped_refptr(), which gets copied into the Closure() and again into the thread when it runs the Closure. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:441: // the order of new operations in this entry. The callers should be totally insensitive to the order of operations. However, I say no post: this should be cheap anyway right?
https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:232: int SimpleEntryImpl::WriteData(int index, I think ReadData / WriteData can be made a lot simpler. How about this: - Always push the Closure onto operations_, regardless of synchronous_entry_in_use_by_worker_. - Always call RunNextOperationIfNeeded. (RunNextOperationIfNeeded should check synchronous_entry_in_use_by_worker_)
https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:222: index, offset, buf, On 2013/04/17 08:17:14, gavinp wrote: > This is really dangerous, calling with the naked buf. The problem is that the > caller can drop their ref to buf, and then buf is deleted under us. The existing > calls used make_scoped_refptr(), which gets copied into the Closure() and again > into the thread when it runs the Closure. Boy this stuff is subtle. I was wrong: I failed to notice that ReadDataInternal's argument is a scoped_refptr<>, so the Closure will copy _THAT_, so it's all good.
https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:89: void SimpleEntryImpl::Doom() { On 2013/04/17 08:12:35, gavinp wrote: > I think doom is one of the only operations that already can be run completely > concurrently with anything else (including other Dooms). Done. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:104: // around the code, to see if we dont call read in a doomed entry. On 2013/04/17 08:12:35, gavinp wrote: > I believe it's correct and valid to Read/Write in a doomed entry. Done. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:232: int SimpleEntryImpl::WriteData(int index, On 2013/04/17 09:23:56, gavinp wrote: > I think ReadData / WriteData can be made a lot simpler. How about this: > > - Always push the Closure onto operations_, regardless of > synchronous_entry_in_use_by_worker_. > - Always call RunNextOperationIfNeeded. > > (RunNextOperationIfNeeded should check synchronous_entry_in_use_by_worker_) Done. https://codereview.chromium.org/14130015/diff/2001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:441: // the order of new operations in this entry. On 2013/04/17 08:17:14, gavinp wrote: > The callers should be totally insensitive to the order of operations. However, I > say no post: this should be cheap anyway right? No post is better because when debugging our minds won't blow.
https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:105: if (operation_running_) { I think if you rebase on top of https://codereview.chromium.org/13880016/ , then this won't be necessary at all; Close() will just be safe to run. Do you think it's a good idea to rebase this on top of that CL, and continue? https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:170: void SimpleEntryImpl::ReadDataInternal(int index, Method ordering: this should go to the end of the file.
https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:105: if (operation_running_) { On 2013/04/17 11:04:36, gavinp wrote: > I think if you rebase on top of https://codereview.chromium.org/13880016/ , then > this won't be necessary at all; Close() will just be safe to run. > > Do you think it's a good idea to rebase this on top of that CL, and continue? If I am going to land before you, I think I should keep it this way and you merge and fix. Even with your CL, I still think it makes sense to run the Close operation in the end of the queue. Although I know your CL solves that problem and allows Close to run at any time, but for the sake of consistency with the idea of a queue of operations, we should run close as the last operation in the queue. Also I think your CL will not be able to CHECK(HasOneRef()) anymore, since we can have multiple Closures enqueued to run later, holding a pointer to |this|. https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:170: void SimpleEntryImpl::ReadDataInternal(int index, On 2013/04/17 11:04:36, gavinp wrote: > Method ordering: this should go to the end of the file. Done.
LGTM. Make sure all try failures are flakes, and give consideration to rerunning them, before landing. https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/15002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:105: if (operation_running_) { On 2013/04/17 11:51:22, felipeg1 wrote: > On 2013/04/17 11:04:36, gavinp wrote: > > I think if you rebase on top of https://codereview.chromium.org/13880016/ , > then > > this won't be necessary at all; Close() will just be safe to run. > > > > Do you think it's a good idea to rebase this on top of that CL, and continue? > > If I am going to land before you, I think I should keep it this way and you > merge and fix. Agreed. And whoever wins the race dodges the merge. Fair point. > Even with your CL, I still think it makes sense to run the Close operation in > the end of the queue. > Although I know your CL solves that problem and allows Close to run at any time, > but for the sake of consistency with the idea of a queue of operations, we > should run close as the last operation in the queue. With my CL, it runs last always, as it's launched from the destructor! So, I agree. > Also > I think your CL will not be able to CHECK(HasOneRef()) anymore, since we can > have multiple Closures enqueued to run later, holding a pointer to |this|. Yeah. Those checks sure didn't live long. https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:89: // Runs next operation in the queue, if any and if there is no other operation Nit: "Runs _the_ next", and also "if and _only_ if". https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:90: // running at the moment. Returns true if a operation has run. What is this return value used for? https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:153: // an operation is pending on the worker pool, the |synchronous_entry_| is While we're here, the last sentence of this comment doesn't make sense anymore. https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:161: std::queue<base::Closure> operations_; pending_operations_ ?
https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:89: // Runs next operation in the queue, if any and if there is no other operation On 2013/04/17 13:13:45, gavinp wrote: > Nit: "Runs _the_ next", and also "if and _only_ if". Done. https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:90: // running at the moment. Returns true if a operation has run. On 2013/04/17 13:13:45, gavinp wrote: > What is this return value used for? It will be used to implement the optimistic writes, you will see. https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:153: // an operation is pending on the worker pool, the |synchronous_entry_| is On 2013/04/17 13:13:45, gavinp wrote: > While we're here, the last sentence of this comment doesn't make sense anymore. Done. https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:161: std::queue<base::Closure> operations_; On 2013/04/17 13:13:45, gavinp wrote: > pending_operations_ ? Done.
FYI, windows bot fails because: No space left on device and mac is a timeout in interactive_ui_tests this should not be related to my change
LGTM, nice stuff https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2122: // The simple cache backend isn't intended to work on windows, which has very s/windows/Windows/, also no need to mention Windows twice in one centence. https://codereview.chromium.org/14130015/diff/30002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/30002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:112: return; since it is not a trivial if->return case, would be more readable to have if (operation_running_) else { ... } https://codereview.chromium.org/14130015/diff/30002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:152: if (index < 0 || index >= kSimpleEntryFileCount || buf_len < 0) was this requested by tests? Otherwise seems like DCHECK would fail earlier and make it easier to investigate.
https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/14130015/diff/19001/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2122: // The simple cache backend isn't intended to work on windows, which has very On 2013/04/17 15:14:46, pasko wrote: > s/windows/Windows/, also no need to mention Windows twice in one centence. Also, windows isn't actually very different from windows.
https://chromiumcodereview.appspot.com/14130015/diff/19001/net/disk_cache/ent... File net/disk_cache/entry_unittest.cc (right): https://chromiumcodereview.appspot.com/14130015/diff/19001/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2122: // The simple cache backend isn't intended to work on windows, which has very On 2013/04/17 15:18:48, gavinp wrote: > On 2013/04/17 15:14:46, pasko wrote: > > s/windows/Windows/, also no need to mention Windows twice in one centence. > > Also, windows isn't actually very different from windows. Done. https://chromiumcodereview.appspot.com/14130015/diff/19001/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2122: // The simple cache backend isn't intended to work on windows, which has very On 2013/04/17 15:14:46, pasko wrote: > s/windows/Windows/, also no need to mention Windows twice in one centence. Done. https://chromiumcodereview.appspot.com/14130015/diff/30002/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14130015/diff/30002/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:112: return; On 2013/04/17 15:14:46, pasko wrote: > since it is not a trivial if->return case, would be more readable to have if > (operation_running_) else { ... } Done. https://chromiumcodereview.appspot.com/14130015/diff/30002/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:152: if (index < 0 || index >= kSimpleEntryFileCount || buf_len < 0) On 2013/04/17 15:14:46, pasko wrote: > was this requested by tests? > Otherwise seems like DCHECK would fail earlier and make it easier to > investigate. Done.
oh, and please add the BUG=
https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:95: DCHECK(!operation_running_); I think this is bad; it's always valid to doom an entry. https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:109: if (operation_running_) { Why is the pattern here so different from ReadData / WriteData? Why not create CloseImpl(), and make this look almost the same as them? https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:258: DCHECK(pending_operations_.size() == 0); DCHECK_NE(0, pending_operations_.size()); https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:259: DCHECK(!operation_running_); + DCHECK(!synchronous_entry); https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:305: data_size_[index] = buf_len; This isn't right; I think this is code leftover from the "fast writes" idea, which should be a separate CL.
https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:95: DCHECK(!operation_running_); On 2013/04/17 16:31:37, gavinp wrote: > I think this is bad; it's always valid to doom an entry. Done. https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:109: if (operation_running_) { On 2013/04/17 16:31:37, gavinp wrote: > Why is the pattern here so different from ReadData / WriteData? Why not create > CloseImpl(), and make this look almost the same as them? Done. https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:258: DCHECK(pending_operations_.size() == 0); On 2013/04/17 16:31:37, gavinp wrote: > DCHECK_NE(0, pending_operations_.size()); You mean: DCHECK_EQ(0U, pending_operations_.size()); https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:259: DCHECK(!operation_running_); On 2013/04/17 16:31:37, gavinp wrote: > + DCHECK(!synchronous_entry); you mean: DCHECK(synchronous_entry); https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:305: data_size_[index] = buf_len; On 2013/04/17 16:31:37, gavinp wrote: > This isn't right; I think this is code leftover from the "fast writes" idea, > which should be a separate CL. Independently if this is a leftover from fast writes, I still believe that here is the place where it should be set. But I am removing for now we put it back when we need
Looks great. The patches didn't apply to trunk in the try bots, so probably just one pull to tip of tree and you're good. LGTM! https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:259: DCHECK(!operation_running_); On 2013/04/17 16:54:59, felipeg1 wrote: > On 2013/04/17 16:31:37, gavinp wrote: > > + DCHECK(!synchronous_entry); > > you mean: > DCHECK(synchronous_entry); Hrrrm. I think I meant DCHECK(!synchronous_entry), but now I'm not sure that's thread safe. If CloseInternal() sets synchronous_entry_ = NULL; the odds are very good that the destructor will run on the IO thread, since we're only running CloseInternal() because of the PostTask from the worker pool. But now I'm not so sure: if we Release() in CloseInternal() but the worker pool loop still holds a ref, we could delete on the IO thread and there may not have been a barrier. So now I think we'd have to do: synchronous_entry_ = NULL; subtle::MemoryBarrier(); Release(); in CloseInternal(). That's... sketchy. So I'm now down to saying we can't assert on this. https://chromiumcodereview.appspot.com/14130015/diff/26005/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:305: data_size_[index] = buf_len; On 2013/04/17 16:54:59, felipeg1 wrote: > On 2013/04/17 16:31:37, gavinp wrote: > > This isn't right; I think this is code leftover from the "fast writes" idea, > > which should be a separate CL. > > Independently if this is a leftover from fast writes, I still believe that here > is the place where it should be set. > But I am removing for now > we put it back when we need Yes, definitely. But data_size_[index] = buf_len won't be right either; more like if (truncate) data_size_[index] = offset + buf_len; else data_size_[index] = std::max(offset + buf_len, data_size_[index]); But that can wait for another CL... :D
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/14130015/55001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/14130015/55003
Message was sent while issue was closed.
Change committed as 194908
Message was sent while issue was closed.
https://codereview.chromium.org/14130015/diff/55003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14130015/diff/55003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:259: DCHECK(pending_operations_.size() == 0); Oops. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
