Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14230009:
Write the Simple Cache Index file to disk once in a while (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. |
DescriptionWrite the Index File to disk after 20 seconds elapsed since the last cache operation has happened.
This will mitigate the need to recover from enumerating all files on disk.
BUG=173394
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194898
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 14
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : sync #Patch Set 7 : rebase with gavin cl #Patch Set 8 : Rebase again #
Messages
Total messages: 15 (0 generated)
Two quick things: Make the one liner a bit shorter, and definitely reference simple backend in it. Fill in the description a bit more, and add the bug number (173394 seems to fit).
Please run a good selection of try bots. It's especially important given the CQ status, but beneficial even with good CQ, since there are more tests in try bots than the CQ. https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:87: io_thread_(io_thread) I like: io_thread_(io_thread) { } best of all the options! https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:301: // The actual IO is asynchronous, so calling WriteToDisk() shouldn't slow down I'd rather rename the method to: "PostWriteToDisk" than have this comment. https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:107: typedef base::Callback<void(scoped_ptr<EntrySet>, bool must_write_to_disk)> Maybe: force_index_flush ? https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:110: void PostponeWriteToDisk(); naming: PostponeWritingToDisk ?
+1 on Gavin's comments too although this mostly looks good to me. https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:87: io_thread_(io_thread) On 2013/04/17 07:54:41, gavinp wrote: > I like: > > io_thread_(io_thread) { > } > > best of all the options! Yes, this is the recommended way. https://codereview.chromium.org/14230009/diff/4001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:299: removed_entries_.clear(); Is this line fixing another bug (which would be covered by a unit test :)?
Can we also make sure to write the index at least once every few minutes?
Overall nice! please fill in BUG=. On 2013/04/17 14:02:14, gavinp wrote: > Can we also make sure to write the index at least once every few minutes? +1, i.e. should not be postponed forever
Done https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:87: io_thread_(io_thread) On 2013/04/17 08:31:39, Philippe wrote: > On 2013/04/17 07:54:41, gavinp wrote: > > I like: > > > > io_thread_(io_thread) { > > } > > > > best of all the options! > > Yes, this is the recommended way. Dude, in another CL someone told me to put {} in the next line. Anyway I also like it more as you said. Done. https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:299: removed_entries_.clear(); On 2013/04/17 08:31:39, Philippe wrote: > Is this line fixing another bug (which would be covered by a unit test :)? Nope. This is not a bug, it is just that we don't need to keep the removed entries after we used it. It is just a waste. https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:301: // The actual IO is asynchronous, so calling WriteToDisk() shouldn't slow down On 2013/04/17 07:54:41, gavinp wrote: > I'd rather rename the method to: "PostWriteToDisk" than have this comment. Done. https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.h:107: typedef base::Callback<void(scoped_ptr<EntrySet>, bool must_write_to_disk)> On 2013/04/17 07:54:41, gavinp wrote: > Maybe: force_index_flush ? Done. https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.h:110: void PostponeWriteToDisk(); On 2013/04/17 07:54:41, gavinp wrote: > naming: PostponeWritingToDisk ? Done.
lgtm https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:87: io_thread_(io_thread) On 2013/04/17 15:01:48, felipeg1 wrote: > On 2013/04/17 08:31:39, Philippe wrote: > > On 2013/04/17 07:54:41, gavinp wrote: > > > I like: > > > > > > io_thread_(io_thread) { > > > } > > > > > > best of all the options! > > > > Yes, this is the recommended way. > > Dude, in another CL someone told me to put {} in the next line. > Anyway I also like it more as you said. > > Done. The style guide says: "The close curly brace is either on the last line by itself or (if other style rules permit) on the same line as the open curly brace." The one on lines 36 and 44 are wrong since: "The open curly brace is always at the end of the same line as the last parameter." https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:310: last_write_to_disk_ = base::Time::Now(); it is already in SimpleIndex::WriteToDisk(), updating it here is confusing
lgtm, thanks Felipe. https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:87: io_thread_(io_thread) On 2013/04/17 15:01:48, felipeg1 wrote: > On 2013/04/17 08:31:39, Philippe wrote: > > On 2013/04/17 07:54:41, gavinp wrote: > > > I like: > > > > > > io_thread_(io_thread) { > > > } > > > > > > best of all the options! > > > > Yes, this is the recommended way. > > Dude, in another CL someone told me to put {} in the next line. > Anyway I also like it more as you said. > > Done. In the other CL I told you to put '}' on the next line, not '{}' :) https://chromiumcodereview.appspot.com/14230009/diff/4001/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:299: removed_entries_.clear(); On 2013/04/17 15:01:48, felipeg1 wrote: > On 2013/04/17 08:31:39, Philippe wrote: > > Is this line fixing another bug (which would be covered by a unit test :)? > > Nope. This is not a bug, it is just that we don't need to keep the removed > entries after we used it. It is just a waste. Great, thanks. https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:28: // WriteToDisk at lest every 5 minutes. Nit: least. https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:324: last_write_to_disk_ = base::Time::Now(); Do you need this given that you already set the field on line 310?
https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:324: last_write_to_disk_ = base::Time::Now(); On 2013/04/17 15:39:18, Philippe wrote: > Do you need this given that you already set the field on line 310? this one should be used instead of the one on line 310, called from elsewhere
https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:310: last_write_to_disk_ = base::Time::Now(); On 2013/04/17 15:38:16, pasko wrote: > it is already in SimpleIndex::WriteToDisk(), updating it here is confusing Done. https://chromiumcodereview.appspot.com/14230009/diff/13001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:324: last_write_to_disk_ = base::Time::Now(); On 2013/04/17 15:39:18, Philippe wrote: > Do you need this given that you already set the field on line 310? Done.
LGTM. I think this needs a rebase. There's some conflicting changes in the CQ too which I don't know who will win the race.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/14230009/38001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/14230009/52001
Message was sent while issue was closed.
Change committed as 194898 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
