Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12277004:
Make SimpleEntryImpl::Close asynchronous. (Closed)
|
Created:
7 years, 10 months ago by gavinp Modified:
7 years, 9 months ago Reviewers:
felipeg_google, rvargas (doing something else), Randy Smith (Not in Mondays), pasko-google - do not use CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, gavinp+disk_chromium.org Visibility:
Public. |
DescriptionMake SimpleEntryImpl::Close asynchronous.
Since only one entry operation can be in flight at once, it's safe to assume that if the IO thread Entry doesn't exist on completion that the sync entry should be deleted.
This issue is downstream of https://codereview.chromium.org/12223075/ and should land after it.
R=rvargas@chromium.org
BUG=173392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185104
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : remediate #
Total comments: 2
Patch Set 5 : rebase #
Total comments: 4
Patch Set 6 : remediation #
Total comments: 6
Patch Set 7 : remediate #
Total comments: 2
Patch Set 8 : remediation to randy's review #
Messages
Total messages: 14 (0 generated)
And the roll continues... https://codereview.chromium.org/12277004/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/12277004/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_entry_impl.h:96: SimpleSynchronousEntry* sync_entry, Funny that this is back! But it's now bound in SimpleEntryImpl rather than being passed back from the SimpleSynchronousEntry.
generally LG, best comments are always about comments. https://codereview.chromium.org/12277004/diff/3001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/3001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:247: // Since at most one PendingEntryOperation can exist at a time for any entry there is no class PendingEntryOperation, better write in plain words: "pending entry operation". The whole sentence contradicts the TODO above about simultaneous reads. Plz, rephrase.
Remediated. https://codereview.chromium.org/12277004/diff/3001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/3001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:247: // Since at most one PendingEntryOperation can exist at a time for any entry On 2013/02/15 17:08:51, pasko wrote: > there is no class PendingEntryOperation, better write in plain words: "pending > entry operation". > > The whole sentence contradicts the TODO above about simultaneous reads. Plz, > rephrase. Done.
https://codereview.chromium.org/12277004/diff/9001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/9001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:248: // Since at present only one pending entry operation can be in flight at a Why write this CL at all instead of fixing the issue once and for all? (as in we still have to support multiple concurrent operation)
+rdsmith Pasko, can you take another look? https://codereview.chromium.org/12277004/diff/9001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/9001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_entry_impl.cc:248: // Since at present only one pending entry operation can be in flight at a On 2013/02/15 23:00:23, rvargas wrote: > Why write this CL at all instead of fixing the issue once and for all? (as in we > still have to support multiple concurrent operation) It's true, we do absolutely have to support multiple concurrent reads. I'd like us to be free of memory leaks before we enter that state, so we can test that this invariant stays true both before and after. This patch fixes a memory leak; that's an improvement, even if it doesn't finish the simple cache, and even if it relies on an invariant that will change soon. There's three of us working on this code (and possibly four soon). Removing obvious problems feels like progress.
LGTM I think it is reasonable to have a more robust version with assumption that we have only one op in flight than having the memory leak. Presence of DCHECKs for more than one op in-flight is also pleasing, though I think at this stage we want to CHECK these during performance tests, would add clarity whether we have perf results in a correct way. https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:248: // Since at present only one pending entry operation can be in flight at a I'd prefer to rephrase: since the simple cache now only supports one pending entry operation in flight .. https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:249: // time, it's safe to now call Close() on |sync|entry|. s/sync[|]entry/sync_entry/
Remediated! rdsmith: ptal! https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:248: // Since at present only one pending entry operation can be in flight at a On 2013/02/26 00:38:19, pasko wrote: > I'd prefer to rephrase: since the simple cache now only supports one pending > entry operation in flight .. Done. https://codereview.chromium.org/12277004/diff/14001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:249: // time, it's safe to now call Close() on |sync|entry|. On 2013/02/26 00:38:19, pasko wrote: > s/sync[|]entry/sync_entry/ Done.
Quick high level question: What does an issue entitled "Blank Page displays for a moment during Web based Sign-in process" and marked as a duplicate have to do with this CL? Typo?
Thank you for the opportunity to learn more about the simple cache; I hope you decide that helping me out that way was worth it :-}. https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:94: DCHECK(synchronous_entry_); I don't see how this change might result in synchronous_entry_ being null at the time of a SimpleEntryImpl::Close(); it seems like you are arranging for synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed before it. https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:252: base::Unretained(sync_entry)), I would feel better about this change if the ownership model for sync_entry was documented either directly or by references somewhere in this class (probably the right place is simple_entry_impl.h:synchronous-entry). Despite the fact of it having perfectly valid uses, base::Unretained() makes me twitch, and so I want to make it as easily as possible to verify that the uses are reasonable ones.
Thanks Randy. I fixed the bug number, too. https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:94: DCHECK(synchronous_entry_); On 2013/02/26 18:47:58, rdsmith wrote: > I don't see how this change might result in synchronous_entry_ being null at the > time of a SimpleEntryImpl::Close(); it seems like you are arranging for > synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed > before it. You're right. This was cruft that got mismerged when an upstream review removed this DCHECK. Thank you. https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:252: base::Unretained(sync_entry)), On 2013/02/26 18:47:58, rdsmith wrote: > I would feel better about this change if the ownership model for sync_entry was > documented either directly or by references somewhere in this class (probably > the right place is simple_entry_impl.h:synchronous-entry). Despite the fact of > it having perfectly valid uses, base::Unretained() makes me twitch, and so I > want to make it as easily as possible to verify that the uses are reasonable > ones. Done.
LGTM; I'm comfortable you understand my points below, and I think your judgement will be better than mine as to whether they should be integrated. (Of course, if you're not comfortable you understand my points, let's do another round :-}.) https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:94: DCHECK(synchronous_entry_); On 2013/02/26 21:00:34, gavinp wrote: > On 2013/02/26 18:47:58, rdsmith wrote: > > I don't see how this change might result in synchronous_entry_ being null at > the > > time of a SimpleEntryImpl::Close(); it seems like you are arranging for > > synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed > > before it. > > You're right. This was cruft that got mismerged when an upstream review removed > this DCHECK. Thank you. Still a little confused--the last patch set still doesn't have the DCHECK. Should it? https://codereview.chromium.org/12277004/diff/23003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/12277004/diff/23003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:123: // completes and finds its owning SimpleEntryImpl has already been destroyed. So the way I read the code / another way to phrase this is that the |synchonous_entry_| ownership is held by the SimpleEntryImpl, but it sometimes passed to the worker thread and then passed back through EntryOperationComplete. The bool synchronous_entry_in_use_by_worker_ indicates when the ownership is out of class, and in those situations the class avoids using synchronous_entry_. Is that right? If it is, would it make sense to signal it with a scoped_ptr<>, and pass that scoped_ptr<> around explicitly so it's easier for code readers to follow the bouncing ownership ball? I'm not dead set on that change, and I'm aware I sometimes go overboard in my use of scoped_ptr<>, but it seems a simpler way to represent what you're trying to do than to have a separate bool.
Thanks for the review Randy, and thanks for taking the time to read this code. I like having more eyes looking at the simple backend. https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/12277004/diff/19001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:94: DCHECK(synchronous_entry_); On 2013/02/26 21:13:11, rdsmith wrote: > On 2013/02/26 21:00:34, gavinp wrote: > > On 2013/02/26 18:47:58, rdsmith wrote: > > > I don't see how this change might result in synchronous_entry_ being null at > > the > > > time of a SimpleEntryImpl::Close(); it seems like you are arranging for > > > synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed > > > before it. > > > > You're right. This was cruft that got mismerged when an upstream review > removed > > this DCHECK. Thank you. > > Still a little confused--the last patch set still doesn't have the DCHECK. > Should it? Aha, I was also confused. In response to this comment, I removed the DCHECK in SimpleEntryImpl::~SimpleEntryImpl. The DCHECK that was here in this CL in earlier revisions isn't needed any more: rebases from upstream changes made it redundant. See my reply to your comment about scoped_ptr<> for more on this topic. https://codereview.chromium.org/12277004/diff/23003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/12277004/diff/23003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:123: // completes and finds its owning SimpleEntryImpl has already been destroyed. On 2013/02/26 21:13:12, rdsmith wrote: > So the way I read the code / another way to phrase this is that the > |synchonous_entry_| ownership is held by the SimpleEntryImpl, but it sometimes > passed to the worker thread and then passed back through EntryOperationComplete. > The bool synchronous_entry_in_use_by_worker_ indicates when the ownership is > out of class, and in those situations the class avoids using synchronous_entry_. > Is that right? > > If it is, would it make sense to signal it with a scoped_ptr<>, and pass that > scoped_ptr<> around explicitly so it's easier for code readers to follow the > bouncing ownership ball? I'm not dead set on that change, and I'm aware I > sometimes go overboard in my use of scoped_ptr<>, but it seems a simpler way to > represent what you're trying to do than to have a separate bool. I like the way you wrote that ownership, so I've fixed the comment to reflect that. Thanks. As for the ownership issues, it's interesting you brought this up. I think there's going to be a need to move to something more like this in the future, but I'm not sure. I am not comfortable using scoped_ptr<> here, because we never call the destructor on |synchronous_entry_|, since it's private. We just call SimpleSynchronousEntry::Close(), and we do that on a worker (since it does blocking filesystem operations). For context, I didn't use scoped_ptr<> earlier, but I did NULL this pointer out to show a lack of ownership in earlier revisions of the simple backend. As well, I made the pending operations themselves objects. See https://codereview.chromium.org/12192005/diff2/21009:26024/net/disk_cache/sim... for the change in which I removed that, and moved to a more C-style usage. Since I basically was close to this scoped_ptr<> idea, and my reviewers talked me away from it, I'm not going to switch to the scoped_ptrs<> right now in response to your comment. However, when we add support for multiple overlapping read operations, things get trickier. I wonder how much data will need to be tracked for that to work properly.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/12277004/25002
Message was sent while issue was closed.
Change committed as 185104 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
