Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22859060:
Fix race condition for non-open/create operations happening after a doom. (Closed)
|
Created:
7 years, 4 months ago by Philippe Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix race condition for non-open/create operations happening after a doom.
Doom used not to be treated as a regular operation in the sense that it didn't
use the pending operations queue. Therefore it was possible to kick off a doom
operation on an entry while another one was happening.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222014
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 21
Patch Set 4 : Address Gavin's and Thomas' comments #
Total comments: 3
Patch Set 5 : Remove code unintentionally relying on undefined behavior (oops :)) #Patch Set 6 : Fix parameters alignment #
Total comments: 1
Patch Set 7 : Add STATE_DOOMED state #Patch Set 8 : Only make doom operations use the pending operations queue #
Total comments: 15
Patch Set 9 : Address Randy's comments #Patch Set 10 : Address Randy's comment #Patch Set 11 : Rebase #
Messages
Total messages: 37 (0 generated)
PTAL :)
reviewers +ttuttle who I know has been looking at this issue.
Right now, SimpleIndex::StartEvictionIfNeeded bypasses the logic in SimpleBackendImpl::IndexReadyForDoom. How should we unify them? Move this logic into the SimpleIndex? As well, this CL only fixes this race for the case where the entry being doomed is open. If the entry is not open, we are in an interesting situation: open will fail (fast-miss), and create should fail (exists if any file exists). But what if the index was wrong, and that entry didn't really exist, then it's possible to create an entry which then gets partially doomed. The next attempt to open it should fail, I guess, and delete those files, leaving us free to create. So I suppose that's fine? https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... net/disk_cache/backend_unittest.cc:2685: entry1->Close(); Why this? https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/entry_unitt... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/entry_unitt... net/disk_cache/entry_unittest.cc:2877: TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic5) { YAY! https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:540: RemoveSelfFromBackend(); There's more to it than this. This was the only callsite other than the destructor for this method; so that means that backend_.get() can never be false; we always have one. But backend_.get() was being used as a proxy for "we're doomed", so things like the use time weren't being updated in the index (look in ReadDataInternal). So we need some other solution for that. Did this pass unit tests on doom? If so, we need a unit test that doomed entries don't update the index! https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:893: PostTaskAndReplyWithResult( Every one of the other calls has a kind of manual expansion of PostTaskAndReplyWithResult, in part I guess because we're handling two separate return values, though not always. I wonder if we should switch every caller to use PostTaskAndReplyWithResult, switch this to be really similar to the others, or leave them a bit inconsistent? I don't have a strong opinion, but I'd like to know what drew you to using this util, rather than cutting and pasting the code from DoomEntry()? https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_operation.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_operation.cc:134: // static And you were so good everywhere else! https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_operation.cc:145: false, All these booleans make me dizzy. Can you make a const bool with the right name for each of them, and use it in the call? I prefer that slightly to /* nameofbool */. I wonder what the right thing to do about all these bools is though. https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_synchronous_entry.h:87: // is meant to be used by the Backend::DoomEntry() call. |callback| will be |callback| ?
Please run more tries on future uploads. Always happy to see a row of green before I tear into the code. :D https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_backend_impl.cc:62: struct DoomEntrySetContext : public base::RefCounted<DoomEntrySetContext> { This probably should be a class, since it inherits. As well, it needs a private explicit destructor, doesn't it?
https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_backend_impl.cc:215: // Used only by mass dooming to execute the client-provided callback only once It is clever that you reuse it as the "single entry has been doomed" callback by binding the entry count to 1, but without a comment to point out that it is reused as such, I assumed it was called once and was confused as to what happens if entries_left isn't 0.
Thanks guys! I suggest Gavin that we do the bigger refactoring in a follow up CL, wdyt? Note that this one is more focused on single-entry (instance) dooming rather than massive dooms. I will update the description to be clearer/more specific. https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/back... File net/disk_cache/backend_unittest.cc (right): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/back... net/disk_cache/backend_unittest.cc:2685: entry1->Close(); On 2013/08/26 20:36:50, gavinp wrote: > Why this? Good question. I needed this (and the change below) to make the CreateEntry assertion below (line 2687) pass. Since the entry wasn't closed before the CreateEntry(), the entry was still active which made CreateEntry() fail. Doom() is poorly specified in the API but to me it shouldn't (even partly) replace a Close(). What do you think? https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... File net/disk_cache/simple/simple_backend_impl.cc (right): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_backend_impl.cc:62: struct DoomEntrySetContext : public base::RefCounted<DoomEntrySetContext> { On 2013/08/26 20:41:37, gavinp wrote: > This probably should be a class, since it inherits. As well, it needs a private > explicit destructor, doesn't it? Yes. In general I use structs when I only use public visibility (regardless of inheritance). https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_backend_impl.cc:215: // Used only by mass dooming to execute the client-provided callback only once On 2013/08/26 21:34:18, ttuttle wrote: > It is clever that you reuse it as the "single entry has been doomed" callback by > binding the entry count to 1, but without a comment to point out that it is > reused as such, I assumed it was called once and was confused as to what happens > if entries_left isn't 0. Yes, good point. Thanks. https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... File net/disk_cache/simple/simple_entry_impl.cc (left): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_entry_impl.cc:540: RemoveSelfFromBackend(); On 2013/08/26 20:36:50, gavinp wrote: > There's more to it than this. > > This was the only callsite other than the destructor for this method; so that > means that backend_.get() can never be false; we always have one. > > But backend_.get() was being used as a proxy for "we're doomed", so things like > the use time weren't being updated in the index (look in ReadDataInternal). > > So we need some other solution for that. > > Did this pass unit tests on doom? If so, we need a unit test that doomed entries > don't update the index! To be honest I'm a bit confused with this MarkAsDoomed() method and the fact that it's re-used in some error code paths in contexts other than dooming. It makes it hard to reason about it and to predict its effects. I'm not sure what you meant by doomed entries should not update the index. Are we not removing them from the index on the line above? I will need you to elaborate :) https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_entry_impl.cc:893: PostTaskAndReplyWithResult( On 2013/08/26 20:36:50, gavinp wrote: > Every one of the other calls has a kind of manual expansion of > PostTaskAndReplyWithResult, in part I guess because we're handling two separate > return values, though not always. > > I wonder if we should switch every caller to use PostTaskAndReplyWithResult, > switch this to be really similar to the others, or leave them a bit > inconsistent? I don't have a strong opinion, but I'd like to know what drew you > to using this util, rather than cutting and pasting the code from DoomEntry()? In my opinion it would be nice to switch to PostTaskAndReplyWithResult() for single-valued return types. It's not high priority though that's why I think we can do the transition incrementally when touching code around. In my opinion, this helper function is more expressive, self-documenting and less hackish. It also makes the bound functions' signature much nicer (by using a return type rather than an output parameter) and also saves a heap allocation. It's also less code to write. What do you think? https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... File net/disk_cache/simple/simple_entry_operation.cc (right): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_entry_operation.cc:134: On 2013/08/26 20:36:50, gavinp wrote: > // static > > And you were so good everywhere else! Ahaha :) https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_entry_operation.cc:145: false, On 2013/08/26 20:36:50, gavinp wrote: > All these booleans make me dizzy. Can you make a const bool with the right name > for each of them, and use it in the call? I prefer that slightly to /* > nameofbool */. I wonder what the right thing to do about all these bools is > though. Yeah I was also pretty sad to have to add this code especially for some state that we don't use (and I don't like to pay for what I don't use :)). And I'm even more sad now with this const variables boilerplate code. This made me think that an alternative design for SimpleEntryOperation (maybe less efficient though due to the extra vptr and heap allocation) would be to have a SimpleEntryOperation abstract base class with the state (e.g. |entry_|, |callback_|) needed by all the subclasses (the subclasses being CreateOperation, OpenOperation...). The fact that we would have to heap-allocate the object would not be a big regression though since we are already doing that to keep track of the current operation in |executing_operation_|. Then we would have a queue of pointers rather than a queue of values. There would be an extra indirection (completely negligible in my opinion) to access the object but there would be less copy on the other hand when the underlying vector is resized. Then there is also the question of how we operate on the concrete type when we are provided with an abstract entry operation. I see two options, my favorite being the second one: - keep the type enumeration and provide a cast template method. - add a visitor interface that simple entry or possibly a private nested delegate class would implement. This would also let us get rid of the operation dispatching code that we have in simple_entry_impl.cc. I would be glad to experiment with this interesting refactoring once we have less high priority tasks (if this ever happens :)) https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... net/disk_cache/simple/simple_synchronous_entry.h:87: // is meant to be used by the Backend::DoomEntry() call. |callback| will be On 2013/08/26 20:36:50, gavinp wrote: > |callback| ? Done.
Adding Randy who looks like the expert on the subject :) Sorry for no consulting you guys earlier. I also came across the race condition with the app cache migration code and started addressing it blindly. Just to be clear, this does not solve all the problems but might be a good thing to start with.
On Tue, Aug 27, 2013 at 11:32 AM, <pliard@chromium.org> wrote: > Adding Randy who looks like the expert on the subject :) Oh, No. I'm Doomed. (:-}) Thanks, Philippe, I'll take a look. -- Randy > Sorry for no consulting > you guys earlier. I also came across the race condition with the app cache > migration code and started addressing it blindly. > > Just to be clear, this does not solve all the problems but might be a good > thing > to start with. > > https://codereview.chromium.**org/22859060/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... net/disk_cache/backend_unittest.cc:2685: entry1->Close(); On 2013/08/27 11:33:30, Philippe wrote: > On 2013/08/26 20:36:50, gavinp wrote: > > Why this? > > Good question. I needed this (and the change below) to make the CreateEntry > assertion below (line 2687) pass. Since the entry wasn't closed before the > CreateEntry(), the entry was still active which made CreateEntry() fail. Doom() > is poorly specified in the API but to me it shouldn't (even partly) replace a > Close(). What do you think? I think this breaks the API; it's legal and should succeed to CreateEntry() while an entry with the same key is open and doomed. https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:540: RemoveSelfFromBackend(); On 2013/08/27 11:33:30, Philippe wrote: > On 2013/08/26 20:36:50, gavinp wrote: > > There's more to it than this. > > > > This was the only callsite other than the destructor for this method; so that > > means that backend_.get() can never be false; we always have one. > > > > But backend_.get() was being used as a proxy for "we're doomed", so things > like > > the use time weren't being updated in the index (look in ReadDataInternal). > > > > So we need some other solution for that. > > > > Did this pass unit tests on doom? If so, we need a unit test that doomed > entries > > don't update the index! > > To be honest I'm a bit confused with this MarkAsDoomed() method and the fact > that it's re-used in some error code paths in contexts other than dooming. It > makes it hard to reason about it and to predict its effects. > > I'm not sure what you meant by doomed entries should not update the index. Are > we not removing them from the index on the line above? I will need you to > elaborate :) There's a member backend_ on SimpleEntryImpl, it's a weakptr. It's zeroed in RemoveSelfFromBackend(). That previously was taken as a signal that the entry was doomed, so that ReadData() for instance wouldn't update the index (really, it couldn't, since the index is accessed through backend_->index()). But now, we're not using backend_ as a weakptr with this change; the only place the weakptr is cleared is in SimpleEntryImpl::~SimpleEntryImpl, so it's really just a naked ptr (except that a entry can outlive its backend object, so it probably still needs to be a weakptr). So both ReadData and writedata have: if (backend.get()) backend_->index()->UseIfExists(entry_hash_); So now, with your change, if we: Create "foo" (entry1) entry1->Doom(); Create "foo" (entry2) entry1->WriteData(); // This should not update the use time in the index. https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:377: removed_key_hashes->size())); What was the undefined behaviour?
https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/backend_uni... net/disk_cache/backend_unittest.cc:2685: entry1->Close(); On 2013/08/27 15:48:02, gavinp wrote: > On 2013/08/27 11:33:30, Philippe wrote: > > On 2013/08/26 20:36:50, gavinp wrote: > > > Why this? > > > > Good question. I needed this (and the change below) to make the CreateEntry > > assertion below (line 2687) pass. Since the entry wasn't closed before the > > CreateEntry(), the entry was still active which made CreateEntry() fail. > Doom() > > is poorly specified in the API but to me it shouldn't (even partly) replace a > > Close(). What do you think? > > I think this breaks the API; it's legal and should succeed to CreateEntry() > while an entry with the same key is open and doomed. Ok, thanks I will address that then :) https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/22859060/diff/5001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:540: RemoveSelfFromBackend(); On 2013/08/27 15:48:02, gavinp wrote: > On 2013/08/27 11:33:30, Philippe wrote: > > On 2013/08/26 20:36:50, gavinp wrote: > > > There's more to it than this. > > > > > > This was the only callsite other than the destructor for this method; so > that > > > means that backend_.get() can never be false; we always have one. > > > > > > But backend_.get() was being used as a proxy for "we're doomed", so things > > like > > > the use time weren't being updated in the index (look in ReadDataInternal). > > > > > > So we need some other solution for that. > > > > > > Did this pass unit tests on doom? If so, we need a unit test that doomed > > entries > > > don't update the index! > > > > To be honest I'm a bit confused with this MarkAsDoomed() method and the fact > > that it's re-used in some error code paths in contexts other than dooming. It > > makes it hard to reason about it and to predict its effects. > > > > I'm not sure what you meant by doomed entries should not update the index. Are > > we not removing them from the index on the line above? I will need you to > > elaborate :) > > There's a member backend_ on SimpleEntryImpl, it's a weakptr. It's zeroed in > RemoveSelfFromBackend(). That previously was taken as a signal that the entry > was doomed, so that ReadData() for instance wouldn't update the index (really, > it couldn't, since the index is accessed through backend_->index()). > > But now, we're not using backend_ as a weakptr with this change; the only place > the weakptr is cleared is in SimpleEntryImpl::~SimpleEntryImpl, so it's really > just a naked ptr (except that a entry can outlive its backend object, so it > probably still needs to be a weakptr). > > So both ReadData and writedata have: > > if (backend.get()) > backend_->index()->UseIfExists(entry_hash_); > > So now, with your change, if we: > > Create "foo" (entry1) > entry1->Doom(); > Create "foo" (entry2) > entry1->WriteData(); // This should not update the use time in the index. > I see now, thanks :) I hadn't understood that the validity of the backend pointer was used to tell whether the entry was doomed (although it's pretty clear now when I read you back). So I intend to add a new DOOM entry state. I would have really appreciated avoiding that but I don't see any other approach, do you?
https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:377: removed_key_hashes->size())); On 2013/08/27 15:48:02, gavinp wrote: > What was the undefined behaviour? +1; this looked fine to me.
https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/22859060/diff/18001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:377: removed_key_hashes->size())); On 2013/08/27 16:44:37, ttuttle wrote: > On 2013/08/27 15:48:02, gavinp wrote: > > What was the undefined behaviour? > > +1; this looked fine to me. Ah thanks Thomas for reminding me this :) So base::Passed(&removed_key_hashes) invalidates |removed_key_hashes| (nothing unexpected so far). But look at this whole PostTaskAndReplyWithResult() expression. |removed_key_hashes| appear twice in two different function call parameter expressions. One of them invalidates |removed_key_hashes| so this was actually relying on the parameters evaluation order :) If the parameters were evaluated from left to right the following code would have produced a nice SIGSEGV since the second use of |removed_key_hashes| is actually dereferencing the pointer (which was nulled by the first use doing a Passed()). Does that make sense?
On 2013/08/27 11:33:29, Philippe wrote: > Thanks guys! > > I suggest Gavin that we do the bigger refactoring in a follow up CL, wdyt? Note > that this one is more focused on single-entry (instance) dooming rather than > massive dooms. I will update the description to be clearer/more specific. [Comment being made without context since I haven't done a full-CL review yet. ] I'd suggest that we not land this CL until we have a clear vision/design as to how we're going to fix the whole thing. I'm in favor of splitting the coding work into multiple CLs, but I'm concerned in the abstract that whatever overall solution we come up with for this problem will require redoing partial fixes (IME synchronization issues usually work that way), so I'd like to finish the design before we start landing pieces of the implementation. Diving into doing a real review now :-} > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/back... > File net/disk_cache/backend_unittest.cc (right): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/back... > net/disk_cache/backend_unittest.cc:2685: entry1->Close(); > On 2013/08/26 20:36:50, gavinp wrote: > > Why this? > > Good question. I needed this (and the change below) to make the CreateEntry > assertion below (line 2687) pass. Since the entry wasn't closed before the > CreateEntry(), the entry was still active which made CreateEntry() fail. Doom() > is poorly specified in the API but to me it shouldn't (even partly) replace a > Close(). What do you think? > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > File net/disk_cache/simple/simple_backend_impl.cc (right): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_backend_impl.cc:62: struct DoomEntrySetContext : > public base::RefCounted<DoomEntrySetContext> { > On 2013/08/26 20:41:37, gavinp wrote: > > This probably should be a class, since it inherits. As well, it needs a > private > > explicit destructor, doesn't it? > > Yes. In general I use structs when I only use public visibility (regardless of > inheritance). > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_backend_impl.cc:215: // Used only by mass dooming > to execute the client-provided callback only once > On 2013/08/26 21:34:18, ttuttle wrote: > > It is clever that you reuse it as the "single entry has been doomed" callback > by > > binding the entry count to 1, but without a comment to point out that it is > > reused as such, I assumed it was called once and was confused as to what > happens > > if entries_left isn't 0. > > Yes, good point. Thanks. > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > File net/disk_cache/simple/simple_entry_impl.cc (left): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_entry_impl.cc:540: RemoveSelfFromBackend(); > On 2013/08/26 20:36:50, gavinp wrote: > > There's more to it than this. > > > > This was the only callsite other than the destructor for this method; so that > > means that backend_.get() can never be false; we always have one. > > > > But backend_.get() was being used as a proxy for "we're doomed", so things > like > > the use time weren't being updated in the index (look in ReadDataInternal). > > > > So we need some other solution for that. > > > > Did this pass unit tests on doom? If so, we need a unit test that doomed > entries > > don't update the index! > > To be honest I'm a bit confused with this MarkAsDoomed() method and the fact > that it's re-used in some error code paths in contexts other than dooming. It > makes it hard to reason about it and to predict its effects. > > I'm not sure what you meant by doomed entries should not update the index. Are > we not removing them from the index on the line above? I will need you to > elaborate :) > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > File net/disk_cache/simple/simple_entry_impl.cc (right): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_entry_impl.cc:893: PostTaskAndReplyWithResult( > On 2013/08/26 20:36:50, gavinp wrote: > > Every one of the other calls has a kind of manual expansion of > > PostTaskAndReplyWithResult, in part I guess because we're handling two > separate > > return values, though not always. > > > > I wonder if we should switch every caller to use PostTaskAndReplyWithResult, > > switch this to be really similar to the others, or leave them a bit > > inconsistent? I don't have a strong opinion, but I'd like to know what drew > you > > to using this util, rather than cutting and pasting the code from DoomEntry()? > > In my opinion it would be nice to switch to PostTaskAndReplyWithResult() for > single-valued return types. It's not high priority though that's why I think we > can do the transition incrementally when touching code around. In my opinion, > this helper function is more expressive, self-documenting and less hackish. It > also makes the bound functions' signature much nicer (by using a return type > rather than an output parameter) and also saves a heap allocation. It's also > less code to write. What do you think? > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > File net/disk_cache/simple/simple_entry_operation.cc (right): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_entry_operation.cc:134: > On 2013/08/26 20:36:50, gavinp wrote: > > // static > > > > And you were so good everywhere else! > > Ahaha :) > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_entry_operation.cc:145: false, > On 2013/08/26 20:36:50, gavinp wrote: > > All these booleans make me dizzy. Can you make a const bool with the right > name > > for each of them, and use it in the call? I prefer that slightly to /* > > nameofbool */. I wonder what the right thing to do about all these bools is > > though. > > Yeah I was also pretty sad to have to add this code especially for some state > that we don't use (and I don't like to pay for what I don't use :)). And I'm > even more sad now with this const variables boilerplate code. > > This made me think that an alternative design for SimpleEntryOperation (maybe > less efficient though due to the extra vptr and heap allocation) would be to > have a SimpleEntryOperation abstract base class with the state (e.g. |entry_|, > |callback_|) needed by all the subclasses (the subclasses being CreateOperation, > OpenOperation...). The fact that we would have to heap-allocate the object would > not be a big regression though since we are already doing that to keep track of > the current operation in |executing_operation_|. Then we would have a queue of > pointers rather than a queue of values. There would be an extra indirection > (completely negligible in my opinion) to access the object but there would be > less copy on the other hand when the underlying vector is resized. > Then there is also the question of how we operate on the concrete type when we > are provided with an abstract entry operation. I see two options, my favorite > being the second one: > - keep the type enumeration and provide a cast template method. > - add a visitor interface that simple entry or possibly a private nested > delegate class would implement. This would also let us get rid of the operation > dispatching code that we have in simple_entry_impl.cc. > > I would be glad to experiment with this interesting refactoring once we have > less high priority tasks (if this ever happens :)) > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > File net/disk_cache/simple/simple_synchronous_entry.h (right): > > https://chromiumcodereview.appspot.com/22859060/diff/5001/net/disk_cache/simp... > net/disk_cache/simple/simple_synchronous_entry.h:87: // is meant to be used by > the Backend::DoomEntry() call. |callback| will be > On 2013/08/26 20:36:50, gavinp wrote: > > |callback| ? > > Done.
PTAL :) I have just added a doomed state to recreate the synchronous entry when a doomed entry instance is being reused in a create entry operation (I hope this is not completely wrong). This is getting more complicated obviously. I don't think I fully see yet the side effects/implications of closing and re-creating the synchronous entry in that particular case. This last patch set adding the doomed state lets avoid breaking the API as I had done in the previous patch sets (maybe you guys have another approach in mind too to avoid that). Gavin, I still need to address your comment about the RemoveSelfFromBackend() issue. I will do that in the next patch set but I wanted to let you guys have a chance to do another iteration today. Randy, I totally agree that we should see the full picture before landing. But to be honest, I have to experiment to see things sometimes :)
Understood about the experimentation, Philippe; go to! But I want to flail around a bit to figure out the more general solution, as I don't think I'm going to be able to do a reasonable job of reviewing this CL without a vision of how it fits into the long-term plan. IIUC, this CL does a couple of things: * Delays completion of the IndexReadyForDoom callback argument until all doom operations requested by it have completed. * For active entries only, make Doom an operation in pending_operations_ like any other, so that it won't continue until all previous operations have completed, and later operations will stack up behind the doom. [Side question: I think this means that Doom (either mass or individual)->Create will from the caller's point of view have an indeterminate result--the create will either succeed or fail depending on whether it arrives before or after the callback from the doom. That right? If so, is it the behavior we want? As a caller, given the interface (which may be broken), I'd like Doom->Create to Just Work. Is that something we should be designing for? Or am I whacked and Create() will just get queued up on the pending_operations_ queue and everything works fine?] The things that this doesn't do: * Mass doom -> Create for inactive entries still races, as before. * Eviction still races with create/open (hadn't thought about the open/evict race (open creates an entry and eviction kills the file after the open succeeds)--that actually sounds bad. Is there something in the code that prevents that from happening? I didn't see it at a quick glance.) The "simple" way to handle this would be to, if you're going to doom an entry, make it active, and then use the basic code in this CL to doom it serially. This would naively involve pinging the worker pool to do the open, but maybe we could come up with a special format of SEI/SSE that's only used for doom, and create it on the IO thread? (I had thought we were worried about the PostTask flood, but it appears we're doing a PostTask() flood already for the active entries, so my concern about designing around that is lower.) If I'm missing something basic, please let me know. https://codereview.chromium.org/22859060/diff/35001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (left): https://codereview.chromium.org/22859060/diff/35001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:278: // is false (i.e. when an operation is not pending on the worker pool). I'd really like to keep a comment here describing the ownership of the SimpleSynchronousEntry. The ownership model isn't simple; SEI owns it, but commits to not deleting it, and the only way it will be deleted is by post to SSE::Close. I'd like to have that sketched out here. (I'd even more like to have it self-documented by passing scoped_ptrs around, but that might require some more complex re-working of the code.)
I don't think the ActiveEntry is the place to solve this. This CL as written has
a really nasty bug. Consider:
entry1 = Open("foo");
int entry1_size = entry1->GetDataSize(0); // assume nonzero
entry1.Doom();
entry2 = Create("foo");
CHECK_EQ(entry1_size, entry1->GetDataSize(0))
<< "Doom does NOT invalidate entries!";
So I don't think any amount of STATE_DOOM fixes this problem. Doom should stay
in the operations queue, to avoid some crazy racy flakes, but we don't need
STATE_DOOM. After a Doom(), just go to STATE_READY.
Instead, when the doom is enqueued, the entry should be removed from
backend_.active_entries_. We probably need to add something like:
std::hash_map<uint64, base::Closure> pending_doom_entries_;
Any time we start an asynchronous doom operation, we put the entry in that set.
If any call to OpenEntry/CreateEntry/DoomEntry comes to the backend while this
operation is pending, we have to enqueue the operation in the Closure; there may
be multiple such operations, so we will end up constructing a list of closures:
void ClosureConcatImpl(const base::Closure& first, const base::Closure second) {
first.Run();
second.Run();
}
base::Closure ClosureConcat(const base::Closure& first, const base::Closure
second) {
return base::Bind(&ConcatImpl, first, second);
}
...
int SimpleBackendImpl::OpenEntry(const std::string& key,
Entry** entry,
const CompletionCallback& callback) {
std::hash_map<uint64, base::Closure>::iterator it = FindBlahForBlah();
if (it != pending_doom_entries_.end()) {
it->second = ClosureConcat(base::Bind(&SimpleBackendImpl::OpenEntry,
AsWeakPtr(), key, entry, callback));
return net::ERR_IO_PENDING;
}
...
When the doom completes, we call the callback on the backend_, which runs the
Closure on pending_doom_entries_, which may or may not be empty (doesn't
matter). All the right callbacks get called, in order, and we can even put DOOMS
in there. Wild.
For Randy's concern about mass dooms, isn't that easy: you put them in
pending_doom_entries_. If any operations come during the mass doom, the same
logic saves us, right?
On 2013/08/27 21:30:04, gavinp wrote:
> I don't think the ActiveEntry is the place to solve this. This CL as written
has
> a really nasty bug. Consider:
>
> entry1 = Open("foo");
> int entry1_size = entry1->GetDataSize(0); // assume nonzero
> entry1.Doom();
> entry2 = Create("foo");
> CHECK_EQ(entry1_size, entry1->GetDataSize(0))
> << "Doom does NOT invalidate entries!";
>
> So I don't think any amount of STATE_DOOM fixes this problem. Doom should stay
> in the operations queue, to avoid some crazy racy flakes, but we don't need
> STATE_DOOM. After a Doom(), just go to STATE_READY.
>
> Instead, when the doom is enqueued, the entry should be removed from
> backend_.active_entries_. We probably need to add something like:
>
> std::hash_map<uint64, base::Closure> pending_doom_entries_;
>
> Any time we start an asynchronous doom operation, we put the entry in that
set.
> If any call to OpenEntry/CreateEntry/DoomEntry comes to the backend while this
> operation is pending, we have to enqueue the operation in the Closure; there
may
> be multiple such operations, so we will end up constructing a list of
closures:
>
> void ClosureConcatImpl(const base::Closure& first, const base::Closure second)
{
> first.Run();
> second.Run();
> }
>
> base::Closure ClosureConcat(const base::Closure& first, const base::Closure
> second) {
> return base::Bind(&ConcatImpl, first, second);
> }
>
> ...
>
> int SimpleBackendImpl::OpenEntry(const std::string& key,
> Entry** entry,
> const CompletionCallback& callback) {
> std::hash_map<uint64, base::Closure>::iterator it = FindBlahForBlah();
> if (it != pending_doom_entries_.end()) {
> it->second = ClosureConcat(base::Bind(&SimpleBackendImpl::OpenEntry,
> AsWeakPtr(), key, entry, callback));
> return net::ERR_IO_PENDING;
> }
> ...
>
>
> When the doom completes, we call the callback on the backend_, which runs the
> Closure on pending_doom_entries_, which may or may not be empty (doesn't
> matter). All the right callbacks get called, in order, and we can even put
DOOMS
> in there. Wild.
>
> For Randy's concern about mass dooms, isn't that easy: you put them in
> pending_doom_entries_. If any operations come during the mass doom, the same
> logic saves us, right?
Randy, we are guaranteed that no operation will be executed on an entry while
it's in the IO_PENDING state. Therefore when a doom is happening, all the
concurrent operations will be enqueued and executed only once the doom completed
(i.e. when the entry moved from the IO_PENDING state). But it's also possible
that I'm missing something :)
Yeah we shouldn't need the DOOM state. I was wrong. I will look into the bug you
mentioned Gavin. Note that I had an approach close to the one you exposed in
patch set 1 (modulo the change in Open() which was wrong). But I thought that
this was some sort of duplication of the active entries concept, that there was
no strong reason to make doom a special case and that maybe it should be aligned
with other operations (I still think it would not be completely bad modulo the
bug(s) that would need to be solved). The nice thing on the other hand with the
backend approach is that it addresses the mass doom problem so from that
perspective at least it looks better.
Wdyt? I will be sheriff/perf sheriff until Monday so I won't be super productive
:/
Ok, I think you convinced me Gavin that keeping doomed entries active is probably not the right approach. I have uploaded a last patch set that only keeps the "pending operations part" of this CL. This should at least address the non-create/open operations race after doom since doom will now be properly serialized. https://chromiumcodereview.appspot.com/22859060/diff/60001/net/disk_cache/ent... File net/disk_cache/entry_unittest.cc (left): https://chromiumcodereview.appspot.com/22859060/diff/60001/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:3012: // the HasOneRef() below. This call can't be optimistic and we are checking Why can't this be optimistic? It is optimistic currently.
On Tuesday, August 27, 2013, wrote:
> I don't think the ActiveEntry is the place to solve this. This CL as
> written has
> a really nasty bug. Consider:
>
> entry1 = Open("foo");
> int entry1_size = entry1->GetDataSize(0); // assume nonzero
> entry1.Doom();
> entry2 = Create("foo");
> CHECK_EQ(entry1_size, entry1->GetDataSize(0))
> << "Doom does NOT invalidate entries!";
>
> So I don't think any amount of STATE_DOOM fixes this problem. Doom should
> stay
> in the operations queue, to avoid some crazy racy flakes, but we don't need
> STATE_DOOM. After a Doom(), just go to STATE_READY.
>
> Instead, when the doom is enqueued, the entry should be removed from
> backend_.active_entries_. We probably need to add something like:
>
> std::hash_map<uint64, base::Closure> pending_doom_entries_;
>
> Any time we start an asynchronous doom operation, we put the entry in that
> set.
> If any call to OpenEntry/CreateEntry/**DoomEntry comes to the backend
> while this
> operation is pending, we have to enqueue the operation in the Closure;
> there may
> be multiple such operations, so we will end up constructing a list of
> closures:
>
> void ClosureConcatImpl(const base::Closure& first, const base::Closure
> second) {
> first.Run();
> second.Run();
> }
>
> base::Closure ClosureConcat(const base::Closure& first, const base::Closure
> second) {
> return base::Bind(&ConcatImpl, first, second);
> }
>
> ...
>
> int SimpleBackendImpl::OpenEntry(**const std::string& key,
> Entry** entry,
> const CompletionCallback& callback) {
> std::hash_map<uint64, base::Closure>::iterator it = FindBlahForBlah();
> if (it != pending_doom_entries_.end()) {
> it->second = ClosureConcat(base::Bind(&**SimpleBackendImpl::OpenEntry,
> AsWeakPtr(), key, entry, callback));
> return net::ERR_IO_PENDING;
> }
> ...
>
>
> When the doom completes, we call the callback on the backend_, which runs
> the
> Closure on pending_doom_entries_, which may or may not be empty (doesn't
> matter). All the right callbacks get called, in order, and we can even put
> DOOMS
> in there. Wild.
>
Gavin: In the clear light of day, I remain confused about the same thing I
was confused about last night. Why not just enqueue the operations on the
entry pending operations queue? You'll need to alter the create/open logic
(maybe by removing the entry from active_entries_) but I don't think you
need another operations queue. Where do you see the need for that?
> For Randy's concern about mass dooms, isn't that easy: you put them in
> pending_doom_entries_. If any operations come during the mass doom, the
> same
> logic saves us, right?
>
Sure, this would save you from having to do a new kind of entry as I
described, which might be a win. Is the issue the trade off between new
kind of entry and new operations queue?
>
>
https://codereview.chromium.**org/22859060/<https://codereview.chromium.org/2...
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On Wednesday, August 28, 2013, wrote: > > Randy, we are guaranteed that no operation will be executed on an entry > while > it's in the IO_PENDING state. Therefore when a doom is happening, all the > concurrent operations will be enqueued and executed only once the doom > completed > (i.e. when the entry moved from the IO_PENDING state). But it's also > possible > that I'm missing something :) > That only works if the entry is active. For mass dooms that include inactive entries, you don't have this protection. Thus my suggestion for making an entry active before dooming it, and finding a way to make an entry active that doesn't involve bouncing off the worker pool for file operations. > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/08/28 18:18:40, rdsmith wrote:
> On Tuesday, August 27, 2013, wrote:
>
> > I don't think the ActiveEntry is the place to solve this. This CL as
> > written has
> > a really nasty bug. Consider:
> >
> > entry1 = Open("foo");
> > int entry1_size = entry1->GetDataSize(0); // assume nonzero
> > entry1.Doom();
> > entry2 = Create("foo");
> > CHECK_EQ(entry1_size, entry1->GetDataSize(0))
> > << "Doom does NOT invalidate entries!";
> >
> > So I don't think any amount of STATE_DOOM fixes this problem. Doom should
> > stay
> > in the operations queue, to avoid some crazy racy flakes, but we don't need
> > STATE_DOOM. After a Doom(), just go to STATE_READY.
> >
> > Instead, when the doom is enqueued, the entry should be removed from
> > backend_.active_entries_. We probably need to add something like:
> >
> > std::hash_map<uint64, base::Closure> pending_doom_entries_;
> >
> > Any time we start an asynchronous doom operation, we put the entry in that
> > set.
> > If any call to OpenEntry/CreateEntry/**DoomEntry comes to the backend
> > while this
> > operation is pending, we have to enqueue the operation in the Closure;
> > there may
> > be multiple such operations, so we will end up constructing a list of
> > closures:
> >
> > void ClosureConcatImpl(const base::Closure& first, const base::Closure
> > second) {
> > first.Run();
> > second.Run();
> > }
> >
> > base::Closure ClosureConcat(const base::Closure& first, const base::Closure
> > second) {
> > return base::Bind(&ConcatImpl, first, second);
> > }
> >
> > ...
> >
> > int SimpleBackendImpl::OpenEntry(**const std::string& key,
> > Entry** entry,
> > const CompletionCallback& callback) {
> > std::hash_map<uint64, base::Closure>::iterator it = FindBlahForBlah();
> > if (it != pending_doom_entries_.end()) {
> > it->second = ClosureConcat(base::Bind(&**SimpleBackendImpl::OpenEntry,
> > AsWeakPtr(), key, entry, callback));
> > return net::ERR_IO_PENDING;
> > }
> > ...
> >
> >
> > When the doom completes, we call the callback on the backend_, which runs
> > the
> > Closure on pending_doom_entries_, which may or may not be empty (doesn't
> > matter). All the right callbacks get called, in order, and we can even put
> > DOOMS
> > in there. Wild.
> >
>
> Gavin: In the clear light of day, I remain confused about the same thing I
> was confused about last night. Why not just enqueue the operations on the
> entry pending operations queue? You'll need to alter the create/open logic
> (maybe by removing the entry from active_entries_) but I don't think you
> need another operations queue. Where do you see the need for that?
I guess you could create an empty active entry. Then you'd have to wire it up so
that it starts up in a state of waiting for doom notification, and make sure
that the entry that's dooming itself sends the notification along correctly.
Seems pretty heavyweight compared to letting the backend know about dooms, and
having it run just one closure?
>
>
> > For Randy's concern about mass dooms, isn't that easy: you put them in
> > pending_doom_entries_. If any operations come during the mass doom, the
> > same
> > logic saves us, right?
> >
>
> Sure, this would save you from having to do a new kind of entry as I
> described, which might be a win. Is the issue the trade off between new
> kind of entry and new operations queue?
>
>
> >
> >
>
https://codereview.chromium.**org/22859060/%3Chttps://codereview.chromium.org...>
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2013/08/28 18:18:40, rdsmith wrote: > On Wednesday, August 28, 2013, wrote: > > > > > Randy, we are guaranteed that no operation will be executed on an entry > > while > > it's in the IO_PENDING state. Therefore when a doom is happening, all the > > concurrent operations will be enqueued and executed only once the doom > > completed > > (i.e. when the entry moved from the IO_PENDING state). But it's also > > possible > > that I'm missing something :) > > > > That only works if the entry is active. For mass dooms that include > inactive entries, you don't have this protection. Thus my suggestion for > making an entry active before dooming it, and finding a way to make an > entry active that doesn't involve bouncing off the worker pool for file > operations. Philippe was speaking specifically to that case; he is not trying to address the out of scope case in this CL. In the case where an entry isn't active, then the entry in pending_doom_entries_ will cause the Create/Open to pickle itself until the doom completes. > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
To illustrate the idea on the simple_backend_impl, I've uploaded https://codereview.chromium.org/23486006/ , which is downstream of this CL and tracks entries that are dooming on the backend. With that CL, we're farther down the path of having mass dooms deflaked too.
https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:261: pending_operations_.push(SimpleEntryOperation::DoomOperation(this, callback)); There's two things about this that might be unnecessarily conservative. 1. Perhaps we should push it to the front of the operations queue, rather than the back? Dooms should happen quickly. 2. It's actually safe to just doom completely asynchronously as long as the state is not STATE_UNINITIALIZED. However, ttuttle's upcoming third-stream CL will add STATE_IO_PENDING to that list when the IO is a write. My thinking is we should do (1), and not (2). WDYT? https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:894: PostTaskAndReplyWithResult( I think there's a bug here, although it's pre-existing. If this entry is already doomed, we don't want to do this, right? I guess that needs a test, too. Nice.
Thanks Gavin. It's good that you have your last CL to address the pre-existing bug. That way we will be able to submit mine pretty soon (I believe) which should make everyone's life easier. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:261: pending_operations_.push(SimpleEntryOperation::DoomOperation(this, callback)); On 2013/08/29 19:31:40, gavinp wrote: > There's two things about this that might be unnecessarily conservative. > > 1. Perhaps we should push it to the front of the operations queue, rather than > the back? Dooms should happen quickly. > > 2. It's actually safe to just doom completely asynchronously as long as the > state is not STATE_UNINITIALIZED. However, ttuttle's upcoming third-stream CL > will add STATE_IO_PENDING to that list when the IO is a write. > > My thinking is we should do (1), and not (2). WDYT? > IMO, it would be a little subtle/risky to re-order the operations in the queue, wdyt? I would favor safety/simplicity here unless this proves to be really problematic. To be honest I wouldn't even have thought about doing this :) https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:894: PostTaskAndReplyWithResult( On 2013/08/29 19:31:40, gavinp wrote: > I think there's a bug here, although it's pre-existing. If this entry is already > doomed, we don't want to do this, right? > > I guess that needs a test, too. Nice. That's your last CL, right?
https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:261: pending_operations_.push(SimpleEntryOperation::DoomOperation(this, callback)); On 2013/08/30 13:43:02, Philippe wrote: > On 2013/08/29 19:31:40, gavinp wrote: > > There's two things about this that might be unnecessarily conservative. > > > > 1. Perhaps we should push it to the front of the operations queue, rather than > > the back? Dooms should happen quickly. > > > > 2. It's actually safe to just doom completely asynchronously as long as the > > state is not STATE_UNINITIALIZED. However, ttuttle's upcoming third-stream CL > > will add STATE_IO_PENDING to that list when the IO is a write. > > > > My thinking is we should do (1), and not (2). WDYT? > > > > IMO, it would be a little subtle/risky to re-order the operations in the queue, > wdyt? I would favor safety/simplicity here unless this proves to be really > problematic. To be honest I wouldn't even have thought about doing this :) You're right. This kind of subtle perf change doesn't belong here. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:894: PostTaskAndReplyWithResult( On 2013/08/30 13:43:02, Philippe wrote: > On 2013/08/29 19:31:40, gavinp wrote: > > I think there's a bug here, although it's pre-existing. If this entry is > already > > doomed, we don't want to do this, right? > > > > I guess that needs a test, too. Nice. > > That's your last CL, right? Yes.
Looking pretty good to me. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (left): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2951: static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef()); Why could/will this have an extra ref? https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:285: // is false (i.e. when an operation is not pending on the worker pool). Suggestion (doesn't have to be part of this CL, just "while we're here"): When I read this, the immediate question that comes to my mind is "Who owns it when executing_operation_ is true?" to which the answer is "no one owns (== can delete) the operation while executing_operation_ is true" and "The SimpleEntryImpl may not be deleted while executing_operation_ is true, as that would leak the SimpleSynchronousEntry". Worth adding those points to this comment? https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.h:88: // code. Suggestion: Now that dooms on active entries are done via the SEI operations queue, it seems weird for this to be a static. I presume it's a static because a) it doesn't need the information in the SSE, b) if the SEI hasn't created an SSE, there's no point in creating one for Doom, and c) we want to keep the SSE info (file descriptors & etc.) around if it's there. I'd value having a comment calling some portion of this out, either here or in the .cc file. That reasonable?
https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (left): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2951: static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef()); On 2013/09/03 18:48:19, rdsmith wrote: > Why could/will this have an extra ref? The doom above is adding an extra reference to the entry instance. This extra reference is removed when the doom completes. So the reference count at this point (if Doom() didn't complete which I can reproduce consistently on my workstation) is 2. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:285: // is false (i.e. when an operation is not pending on the worker pool). On 2013/09/03 18:48:19, rdsmith wrote: > Suggestion (doesn't have to be part of this CL, just "while we're here"): When I > read this, the immediate question that comes to my mind is "Who owns it when > executing_operation_ is true?" to which the answer is "no one owns (== can > delete) the operation while executing_operation_ is true" and "The > SimpleEntryImpl may not be deleted while executing_operation_ is true, as that > would leak the SimpleSynchronousEntry". Worth adding those points to this > comment? Thanks. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.h:88: // code. On 2013/09/03 18:48:19, rdsmith wrote: > Suggestion: Now that dooms on active entries are done via the SEI operations > queue, it seems weird for this to be a static. I presume it's a static because > a) it doesn't need the information in the SSE, b) if the SEI hasn't created an > SSE, there's no point in creating one for Doom, and c) we want to keep the SSE > info (file descriptors & etc.) around if it's there. I'd value having a comment > calling some portion of this out, either here or in the .cc file. That > reasonable? Thanks Randy. I find that the current comment wasn't providing much value. For instance it said 'Be careful! This is meant to be used by the Backend::DoomEntry()'. Not very useful to me if it doesn't say why I should be careful :) I replaced it with a simple sentence that should cover/imlpy a few points.
LGTM. https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.h:88: // code. On 2013/09/04 09:35:23, Philippe wrote: > On 2013/09/03 18:48:19, rdsmith wrote: > > Suggestion: Now that dooms on active entries are done via the SEI operations > > queue, it seems weird for this to be a static. I presume it's a static > because > > a) it doesn't need the information in the SSE, b) if the SEI hasn't created an > > SSE, there's no point in creating one for Doom, and c) we want to keep the SSE > > info (file descriptors & etc.) around if it's there. I'd value having a > comment > > calling some portion of this out, either here or in the .cc file. That > > reasonable? > > Thanks Randy. I find that the current comment wasn't providing much value. For > instance it said 'Be careful! This is meant to be used by the > Backend::DoomEntry()'. Not very useful to me if it doesn't say why I should be > careful :) I replaced it with a simple sentence that should cover/imlpy a few > points. Awesome; thank you. Just because I'm a (maybe too :-}) explicit person, willing to add " ... if any (allowing operations to continue to be executed through that instance)."? Just a suggestion; feel free not to if you feel it's too verbose/doesn't add value.
Thanks Randy! https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/22859060/diff/60001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.h:88: // code. On 2013/09/04 15:44:16, rdsmith wrote: > On 2013/09/04 09:35:23, Philippe wrote: > > On 2013/09/03 18:48:19, rdsmith wrote: > > > Suggestion: Now that dooms on active entries are done via the SEI operations > > > queue, it seems weird for this to be a static. I presume it's a static > > because > > > a) it doesn't need the information in the SSE, b) if the SEI hasn't created > an > > > SSE, there's no point in creating one for Doom, and c) we want to keep the > SSE > > > info (file descriptors & etc.) around if it's there. I'd value having a > > comment > > > calling some portion of this out, either here or in the .cc file. That > > > reasonable? > > > > Thanks Randy. I find that the current comment wasn't providing much value. For > > instance it said 'Be careful! This is meant to be used by the > > Backend::DoomEntry()'. Not very useful to me if it doesn't say why I should be > > careful :) I replaced it with a simple sentence that should cover/imlpy a few > > points. > > Awesome; thank you. Just because I'm a (maybe too :-}) explicit person, willing > to add " ... if any (allowing operations to continue to be executed through that > instance)."? Just a suggestion; feel free not to if you feel it's too > verbose/doesn't add value. Done.
We identified a few tests we needed while hacking at this CL, right? What's the status on those?
On 2013/09/05 21:12:44, gavinp wrote: > We identified a few tests we needed while hacking at this CL, right? What's the > status on those? The scope of this CL totally changed after we realized that keeping doomed entries active was not the right approach. This CL is now only covering entry-level/non-backend-operations (i.e. non-create/open operations) that follow a doom operation. So the unit test doing a create after a doom and checking the entry size would still fail/be flaky (but would be a perfect fit for your CL IMO). I had pinged you a link (/home/pliard/doom_unit_tests.patch) to a patch in your CL. The other unit test was about making sure that doomed entries don't update the index but this also implies some backend operations after a doom. What do you think? I can prepare another CL that would need your CL to be submitted before we can land it.
On 2013/09/06 08:33:35, Philippe wrote: > On 2013/09/05 21:12:44, gavinp wrote: > > We identified a few tests we needed while hacking at this CL, right? What's > the > > status on those? > > The scope of this CL totally changed after we realized that keeping doomed > entries active was not the right approach. This CL is now only covering > entry-level/non-backend-operations (i.e. non-create/open operations) that follow > a doom operation. So the unit test doing a create after a doom and checking the > entry size would still fail/be flaky (but would be a perfect fit for your CL > IMO). I had pinged you a link (/home/pliard/doom_unit_tests.patch) to a patch in > your CL. The other unit test was about making sure that doomed entries don't > update the index but this also implies some backend operations after a doom. > > What do you think? I can prepare another CL that would need your CL to be > submitted before we can land it. Aha, thanks for the reminder. This CL turns back on the tests it fixes. Right. So I'll go through one more time for review. Thanks for answering my silly question.
LGTM. Thanks!
Thanks Gavin!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/22859060/92001
Message was sent while issue was closed.
Change committed as 222014 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
