Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13731002:
Cache Backend Proxy to intercept all cache events from the IO thread. (Closed)
|
Created:
7 years, 8 months ago by pasko-google - do not use Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, felipeg_google Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCache Backend Proxy to intercept all cache events from the IO thread.
Added a build flag to link with the tracing backend, tests may use it without a
flag. Recording the actual event trace is TBD. Added a test that runs on both
BackendImpl and SimpleBackendImpl.
BUG=173384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193652
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193958
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194173
Patch Set 1 #Patch Set 2 : added destructor #
Total comments: 49
Patch Set 3 : addressed review comments from patch set 2 #Patch Set 4 : removed the USE_TRACING_CACHE_BACKEND check from tests #Patch Set 5 : uploading again, some net.gyp changes were not picked up #Patch Set 6 : actually picking up the net.gyp change #
Total comments: 10
Patch Set 7 : addressed comments in patch set 6 #Patch Set 8 : rebased #Patch Set 9 : disable integrity checking on the Simple Backend test #Patch Set 10 : disable the test on windows #
Total comments: 4
Patch Set 11 : memset0 SimpleFileHeader #Patch Set 12 : trivial net.gyp merge #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/backend_uni... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/backend_uni... net/disk_cache/backend_unittest.cc:2699: #ifdef USE_TRACING_CACHE_BACKEND This has no relation with the active defines for the net target, so it seems pointless. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/in_flight_b... File net/disk_cache/in_flight_backend_io.h (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/in_flight_b... net/disk_cache/in_flight_backend_io.h:108: private: nit: move one line down (emty line before, not after). https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:26: class EntryProxy : public Entry, don't declare a class in the middle of the definition of another class. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:30: EntryProxy(Entry *entry, TracingCacheBackend* be) nit: backend https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:43: virtual std::string GetKey() const OVERRIDE { why not track everything? To avoid overhead while recording? https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:57: int64 start_time = base::TimeTicks::Now().ToInternalValue(); separate class definition from class declaration. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:64: base::Bind(&EntryProxy::EntryOpComplete, base::Unretained(this), nit: indentation https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:64: base::Bind(&EntryProxy::EntryOpComplete, base::Unretained(this), Cannot use Unretained here https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:91: return entry_->ReadSparseData(offset, buf, buf_len, callback); definitely track. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:148: Entry** entry, Why Entry** and not Entry* (this method should not modify the value) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:154: EntryProxy* e; nit: don't use single letter variable names (for anything not trivial) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:156: e = open_entries_[entry]; don't search twice https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:191: DCHECK(entry == NULL || *entry == NULL); why *entry == null ? that's not part of the contract. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:192: return base::Bind(&TracingCacheBackend::BackendOpComplete, nit: A method with a single line of implementation looks like an overkill... how about at least moving start_time here :) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:193: base::Unretained(this), start_time, op, key, entry, cb); nit: indent under first arg https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:214: int64 start_time = base::TimeTicks::Now().ToInternalValue(); why are you using int64 instead of Time? https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:216: key, entry, BindCompletion(BackendIO::OP_CREATE, start_time, key, entry, BackendIO is going away... I would not recommend using it just to reuse an enum (which would be questionable even if the class were not going away). https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:234: Entry* e; ditto https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:241: return backend_->DoomAllEntries(callback); why are we not tracking this? (and the other calls) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.h (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.h:17: class NET_EXPORT TracingCacheBackend : public Backend, what does this class do? https://codereview.chromium.org/13731002/diff/5001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/13731002/diff/5001/net/net.gyp#newcode2020 net/net.gyp:2020: 'disk_cache/tracing_cache_backend.cc', I don't like adding the same files to two different targets... especially if they may end up compiled into the same executable. Why not just add the files to net all the time? You could still add the define based on a gyp variable if you want.
Ricardo, I am responding now with no changes in code, addressing the bigger discussions. I am going to address lower-level comments in a "trivial way" later today along with the code. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/backend_uni... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/backend_uni... net/disk_cache/backend_unittest.cc:2699: #ifdef USE_TRACING_CACHE_BACKEND On 2013/04/05 19:25:13, rvargas wrote: > This has no relation with the active defines for the net target, so it seems > pointless. This guards against someone trying to run tests with the tracing backend always on, which may reveal obscure bugs that I do not want anyone to step in. I am OK removing it or enhancing with a comment that would clarify the intentions. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:43: virtual std::string GetKey() const OVERRIDE { On 2013/04/05 19:25:13, rvargas wrote: > why not track everything? To avoid overhead while recording? Yes, I did not measure the CPU/memory overhead yet. We'd keep all events in memory until they are flushed, which on memory-constrained systems may be an issue. More reasoning behind lack of TODOs: It is not quite interesting to see how much time is spent on trivial operations. As the first approximation I want to track only he operations that go down to disk access. I just did not want to add TODO clutter, this change is rather about backend interface and main internal interception mechanisms, I am leaving out simple details now on purpose to simplify the review, make it more gradual than one huge bulk. I am OK adding the TODOs back if it is more comfortable to review this way. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:216: key, entry, BindCompletion(BackendIO::OP_CREATE, start_time, key, entry, On 2013/04/05 19:25:13, rvargas wrote: > BackendIO is going away... I would not recommend using it just to reuse an enum > (which would be questionable even if the class were not going away). I could create a different enum, though some enum constants have confusing name differences, such as: OP_DOOM, OP_DOOM_ENTRY. I thought that they might get renamed some day, and wanted to make sure the renaming gets propagated. It is not clear how it is going away, the enum might even stay as it is, and I am not reusing anything beyond the enum. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:241: return backend_->DoomAllEntries(callback); On 2013/04/05 19:25:13, rvargas wrote: > why are we not tracking this? (and the other calls) I was not measuring it in the initial experiments since the simple backend does not implement it. Should do. I am adding a TODO. https://codereview.chromium.org/13731002/diff/5001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/13731002/diff/5001/net/net.gyp#newcode2020 net/net.gyp:2020: 'disk_cache/tracing_cache_backend.cc', On 2013/04/05 19:25:13, rvargas wrote: > I don't like adding the same files to two different targets... especially if > they may end up compiled into the same executable. > > Why not just add the files to net all the time? You could still add the define > based on a gyp variable if you want. The reason was to save on compiled binary size. There is going to be 2 more files which may even deserve a static library that we could link in conditionally. WDYT? Would that look better? As you suggest we would pay the price for the binary size to avoid repeating the files twice in the gyp file, but we still would not get the benefit of being able to turn it on at runtime. Am I right? I do not like this specific set of limitations.
https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:216: key, entry, BindCompletion(BackendIO::OP_CREATE, start_time, key, entry, On 2013/04/08 15:37:40, pasko wrote: > On 2013/04/05 19:25:13, rvargas wrote: > > BackendIO is going away... I would not recommend using it just to reuse an > enum > > (which would be questionable even if the class were not going away). > > I could create a different enum, though some enum constants have confusing name > differences, such as: OP_DOOM, OP_DOOM_ENTRY. I thought that they might get ... and that show that the constants were never meant to be used outside of the owner class, hence being private. > renamed some day, and wanted to make sure the renaming gets propagated. It is why would they have to be propagated? The enum is not a canonical list of the operations that a backend may need to do... it was just the list of the things the in_flight_backend_io knows about. So yes, please don't reuse the enum and define one for the async things needed by the tracer (and private to it :) ) > not clear how it is going away, the enum might even stay as it is, and I am not > reusing anything beyond the enum. https://codereview.chromium.org/13731002/diff/5001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/13731002/diff/5001/net/net.gyp#newcode2020 net/net.gyp:2020: 'disk_cache/tracing_cache_backend.cc', On 2013/04/08 15:37:40, pasko wrote: > On 2013/04/05 19:25:13, rvargas wrote: > > I don't like adding the same files to two different targets... especially if > > they may end up compiled into the same executable. > > > > Why not just add the files to net all the time? You could still add the define > > based on a gyp variable if you want. > > The reason was to save on compiled binary size. There is going to be 2 more Saving binary size should be the job of the linker. If nobody uses the class on a regular build, the code should be optimized out. > files which may even deserve a static library that we could link in > conditionally. WDYT? Would that look better? please don't do that. 3 files don't (generally) make a static library. Static libraries increase complexity of gyp and or increase the number of dynamic libraries that we have to build for the components build... which is not a good thing. In this specific case, that would mean net depending on a library that depends on net... > > As you suggest we would pay the price for the binary size to avoid repeating the > files twice in the gyp file, but we still would not get the benefit of being > able to turn it on at runtime. Am I right? I do not like this specific set of > limitations. There should be no binary size issue when the code is not used... and it solves the issue of including the same code multiple times when building tools... plus the funky issue with the define in unit tests that attempts to catch errors (without success because nothing prevents the definition of USE_TRACING_CACHE_BACKEND for net while that definition is invisible to unit tests). I think that trying to add or remove files with gyp to various targets depending on gyp variables only make things more complicated and error prone. To be clear, just removing the files from net would be fine, but once we need to add them back into the test executable we enter murky waters.
https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/in_flight_b... File net/disk_cache/in_flight_backend_io.h (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/in_flight_b... net/disk_cache/in_flight_backend_io.h:108: private: On 2013/04/05 19:25:13, rvargas wrote: > nit: move one line down (emty line before, not after). Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:26: class EntryProxy : public Entry, On 2013/04/05 19:25:13, rvargas wrote: > don't declare a class in the middle of the definition of another class. Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:30: EntryProxy(Entry *entry, TracingCacheBackend* be) On 2013/04/05 19:25:13, rvargas wrote: > nit: backend Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:57: int64 start_time = base::TimeTicks::Now().ToInternalValue(); On 2013/04/05 19:25:13, rvargas wrote: > separate class definition from class declaration. Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:64: base::Bind(&EntryProxy::EntryOpComplete, base::Unretained(this), On 2013/04/05 19:25:13, rvargas wrote: > nit: indentation Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:91: return entry_->ReadSparseData(offset, buf, buf_len, callback); On 2013/04/05 19:25:13, rvargas wrote: > definitely track. Added a TODO. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:148: Entry** entry, On 2013/04/05 19:25:13, rvargas wrote: > Why Entry** and not Entry* (this method should not modify the value) Just copied arguments from another function without thinking. Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:154: EntryProxy* e; On 2013/04/05 19:25:13, rvargas wrote: > nit: don't use single letter variable names (for anything not trivial) Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:156: e = open_entries_[entry]; On 2013/04/05 19:25:13, rvargas wrote: > don't search twice Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:191: DCHECK(entry == NULL || *entry == NULL); On 2013/04/05 19:25:13, rvargas wrote: > why *entry == null ? that's not part of the contract. I was checking this invariant, but seems like it is no longer needed. Removed. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:192: return base::Bind(&TracingCacheBackend::BackendOpComplete, On 2013/04/05 19:25:13, rvargas wrote: > nit: A method with a single line of implementation looks like an overkill... how > about at least moving start_time here :) This would not work for, say, OpenEntry that returns synchronously. It save lines in the end because the amount of characters is smaller :) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:193: base::Unretained(this), start_time, op, key, entry, cb); On 2013/04/05 19:25:13, rvargas wrote: > nit: indent under first arg Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:214: int64 start_time = base::TimeTicks::Now().ToInternalValue(); On 2013/04/05 19:25:13, rvargas wrote: > why are you using int64 instead of Time? Because passing an object by value feels icky. Sure the compiler would probably optimize it into copying the single field, but it is easier to understand what the parameter is when it is just a 64bit timestamp. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:216: key, entry, BindCompletion(BackendIO::OP_CREATE, start_time, key, entry, On 2013/04/08 18:28:34, rvargas wrote: > On 2013/04/08 15:37:40, pasko wrote: > > On 2013/04/05 19:25:13, rvargas wrote: > > > BackendIO is going away... I would not recommend using it just to reuse an > > enum > > > (which would be questionable even if the class were not going away). > > > > I could create a different enum, though some enum constants have confusing > name > > differences, such as: OP_DOOM, OP_DOOM_ENTRY. I thought that they might get > > ... and that show that the constants were never meant to be used outside of the > owner class, hence being private. > > > > renamed some day, and wanted to make sure the renaming gets propagated. It is > > why would they have to be propagated? The enum is not a canonical list of the > operations that a backend may need to do... it was just the list of the things > the in_flight_backend_io knows about. > > So yes, please don't reuse the enum and define one for the async things needed > by the tracer (and private to it :) ) I created an independent enum. Just copied. Some of the operations are unused right now, will be used later as these values will be saved in the in-memory representation of the trace events. So let's wait for the coming CLs to discuss their exact look. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:234: Entry* e; On 2013/04/05 19:25:13, rvargas wrote: > ditto Done. https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.h (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.h:17: class NET_EXPORT TracingCacheBackend : public Backend, On 2013/04/05 19:25:13, rvargas wrote: > what does this class do? Done. https://codereview.chromium.org/13731002/diff/5001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/13731002/diff/5001/net/net.gyp#newcode2020 net/net.gyp:2020: 'disk_cache/tracing_cache_backend.cc', On 2013/04/08 18:28:34, rvargas wrote: > On 2013/04/08 15:37:40, pasko wrote: > > On 2013/04/05 19:25:13, rvargas wrote: > > > I don't like adding the same files to two different targets... especially if > > > they may end up compiled into the same executable. > > > > > > Why not just add the files to net all the time? You could still add the > define > > > based on a gyp variable if you want. > > > > The reason was to save on compiled binary size. There is going to be 2 more > > Saving binary size should be the job of the linker. If nobody uses the class on > a regular build, the code should be optimized out. I am relying on the linker now. Thanks. Generally it is not ideal: it takes only two non-careful engineers to reuse a constant from the file and suddenly the whole set of heavy files is linked in. It is not easy to write a test against unwanted linkages, so in the end a "trivial one-liner" pulls in too much. On the other hand if owners in net/ are used to watching on accidental reuses in previously optional files, everything is fine. I just did not assume reviewers are as careful.
Almost there https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:192: return base::Bind(&TracingCacheBackend::BackendOpComplete, On 2013/04/09 11:53:17, pasko wrote: > On 2013/04/05 19:25:13, rvargas wrote: > > nit: A method with a single line of implementation looks like an overkill... > how > > about at least moving start_time here :) > > This would not work for, say, OpenEntry that returns synchronously. It save What would not work? > lines in the end because the amount of characters is smaller :) https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:214: int64 start_time = base::TimeTicks::Now().ToInternalValue(); On 2013/04/09 11:53:17, pasko wrote: > On 2013/04/05 19:25:13, rvargas wrote: > > why are you using int64 instead of Time? > > Because passing an object by value feels icky. Sure the compiler would probably > optimize it into copying the single field, but it is easier to understand what > the parameter is when it is just a 64bit timestamp. That's not the convention on Chrome. Time was specifically designed to be passed around. https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:40: typedef TracingCacheBackend::Operation Operation; do you need this? https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:53: RwOpExtra extra, const CompletionCallback& cb, int rv); nit: if it's the same to you, invert the order of int result and the callback (yes, don't use rv on an argument list) https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:259: key, entry, BindCompletion(OP_CREATE, start_time, key, entry, nit: first two args on the previous line (as OpenEntry) https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... File net/disk_cache/tracing_cache_backend.h (right): https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.h:56: OP_OPEN_PREV, what's this? :) https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.h:61: OP_FLUSH_QUEUE, ditto (etc). This is a good place to only have what you intend to track.
https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:192: return base::Bind(&TracingCacheBackend::BackendOpComplete, On 2013/04/09 18:54:01, rvargas wrote: > On 2013/04/09 11:53:17, pasko wrote: > > On 2013/04/05 19:25:13, rvargas wrote: > > > nit: A method with a single line of implementation looks like an overkill... > > how > > > about at least moving start_time here :) > > > > This would not work for, say, OpenEntry that returns synchronously. It save > > What would not work? in OpenEntry() we use start_time for two cases: 1. pending operation: should pass the start_time to the callback firing at op completion, so it gets passed to BindCompletion() 2. synchronous operation: should pass the start_time directly to RecordEvent() We have a value consumed by both BindCompletion() and RecordEvent(), so we cannot move its creation to BindCompletion(). https://codereview.chromium.org/13731002/diff/5001/net/disk_cache/tracing_cac... net/disk_cache/tracing_cache_backend.cc:214: int64 start_time = base::TimeTicks::Now().ToInternalValue(); On 2013/04/09 18:54:01, rvargas wrote: > On 2013/04/09 11:53:17, pasko wrote: > > On 2013/04/05 19:25:13, rvargas wrote: > > > why are you using int64 instead of Time? > > > > Because passing an object by value feels icky. Sure the compiler would > probably > > optimize it into copying the single field, but it is easier to understand what > > the parameter is when it is just a 64bit timestamp. > > That's not the convention on Chrome. Time was specifically designed to be passed > around. Done. https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... File net/disk_cache/tracing_cache_backend.cc (right): https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:40: typedef TracingCacheBackend::Operation Operation; On 2013/04/09 18:54:01, rvargas wrote: > do you need this? Makes referencing some parameter types a little shorter. https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:53: RwOpExtra extra, const CompletionCallback& cb, int rv); On 2013/04/09 18:54:01, rvargas wrote: > nit: if it's the same to you, invert the order of int result and the callback > (yes, don't use rv on an argument list) I am confused, this function gets curried down to a net::CompletionCallback, the result should be the last argument. I am renaming rv -> result, if it is what you want. https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.cc:259: key, entry, BindCompletion(OP_CREATE, start_time, key, entry, On 2013/04/09 18:54:01, rvargas wrote: > nit: first two args on the previous line (as OpenEntry) Done. https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... File net/disk_cache/tracing_cache_backend.h (right): https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.h:56: OP_OPEN_PREV, On 2013/04/09 18:54:01, rvargas wrote: > what's this? :) seems like an outdated operation not invoked for the current backend https://codereview.chromium.org/13731002/diff/19001/net/disk_cache/tracing_ca... net/disk_cache/tracing_cache_backend.h:61: OP_FLUSH_QUEUE, On 2013/04/09 18:54:01, rvargas wrote: > ditto (etc). This is a good place to only have what you intend to track. I do not like putting the decision about the set of supported operations in this CL because it is not hooked in the code, hence the intentions are not 100% clear. So I am now leaving here only the values that are needed for this specific CL and will extend later as more tracing arrives.
lgtm
Ricardo, thank you for review. I made an additional one-line change to disable the integrity checking on the Simple Backend case. Integrity check failure occurred after we started sharing the directory for the both backends. I think it is good that it failed, nice reminder. I've put a TODO to implement integrity checking in the Simple Backend. About to commit now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/13731002/35001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/13731002/35001
Message was sent while issue was closed.
Change committed as 193652
Message was sent while issue was closed.
Looks like the valgrind bot detected some uninitialized reads: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... I haven't analyzed it yet. The report: Use of uninitialised value of size 8 crc32_little (third_party/zlib/crc32.c:281) MOZ_Z_crc32 (third_party/zlib/crc32.c:239) disk_cache::SimpleIndex::Serialize(std::basic_string<char, std::char_traits<char>, std::allocator<char> >*) (net/disk_cache/simple/simple_index.cc:233) disk_cache::SimpleIndex::Cleanup() (net/disk_cache/simple/simple_index.cc:240) disk_cache::SimpleBackendImpl::~SimpleBackendImpl() (net/disk_cache/simple/simple_backend_impl.cc:61) disk_cache::SimpleBackendImpl::~SimpleBackendImpl() (net/disk_cache/simple/simple_backend_impl.cc:62) base::DefaultDeleter<disk_cache::Backend>::operator()(disk_cache::Backend*) const (./base/memory/scoped_ptr.h:138) base::internal::scoped_ptr_impl<disk_cache::Backend, base::DefaultDeleter<disk_cache::Backend> >::~scoped_ptr_impl() (./base/memory/scoped_ptr.h:221) scoped_ptr<disk_cache::Backend, base::DefaultDeleter<disk_cache::Backend> >::~scoped_ptr() (./base/memory/scoped_ptr.h:311) disk_cache::TracingCacheBackend::~TracingCacheBackend() (net/disk_cache/tracing_cache_backend.cc:183) disk_cache::TracingCacheBackend::~TracingCacheBackend() (net/disk_cache/tracing_cache_backend.cc:184) DiskCacheTestWithCache::TearDown() (net/disk_cache/disk_cache_test_base.cc:228)
Message was sent while issue was closed.
Thanks! I am looking into this. On 2013/04/11 16:56:30, Reid Kleckner wrote: > Looks like the valgrind bot detected some uninitialized reads: > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%2520Tests%2520... > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%2520Tests%2520... > > I haven't analyzed it yet. > > The report: > > Use of uninitialised value of size 8 > crc32_little (third_party/zlib/crc32.c:281) > MOZ_Z_crc32 (third_party/zlib/crc32.c:239) > disk_cache::SimpleIndex::Serialize(std::basic_string<char, > std::char_traits<char>, std::allocator<char> >*) > (net/disk_cache/simple/simple_index.cc:233) > disk_cache::SimpleIndex::Cleanup() (net/disk_cache/simple/simple_index.cc:240) > disk_cache::SimpleBackendImpl::~SimpleBackendImpl() > (net/disk_cache/simple/simple_backend_impl.cc:61) > disk_cache::SimpleBackendImpl::~SimpleBackendImpl() > (net/disk_cache/simple/simple_backend_impl.cc:62) > base::DefaultDeleter<disk_cache::Backend>::operator()(disk_cache::Backend*) > const (./base/memory/scoped_ptr.h:138) > base::internal::scoped_ptr_impl<disk_cache::Backend, > base::DefaultDeleter<disk_cache::Backend> >::~scoped_ptr_impl() > (./base/memory/scoped_ptr.h:221) > scoped_ptr<disk_cache::Backend, base::DefaultDeleter<disk_cache::Backend> > >::~scoped_ptr() (./base/memory/scoped_ptr.h:311) > disk_cache::TracingCacheBackend::~TracingCacheBackend() > (net/disk_cache/tracing_cache_backend.cc:183) > disk_cache::TracingCacheBackend::~TracingCacheBackend() > (net/disk_cache/tracing_cache_backend.cc:184) > DiskCacheTestWithCache::TearDown() > (net/disk_cache/disk_cache_test_base.cc:228)
Message was sent while issue was closed.
the serialization issue is resolved in another change: https://codereview.chromium.org/14204002 disabled the test on windows to avoid flake on the index file
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/13731002/54001
Message was sent while issue was closed.
Change committed as 193958
Message was sent while issue was closed.
https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2703: // File renaming is flaky on Windows, disable all Simple Backend Tests there. I know Windows is not your priority, but I'm not convinced by this explanation. Could you clarify that for me / file the appropriate bugs to track and fix whatever is broken? https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2705: #define TEST_SIMPLECACHE_F(c, t) TEST_F(c, DISABLED_##t) Is your intention to disable all simple cache tests for windows? If the code is not intended to work in one platform, maybe it is better not to add noise to the tests (disabled tests are generally a bug that should be fix asap) and ifdef the block of tests instead (not a bad idea, considering that you guys are not implementing everything for all platforms yet)
Message was sent while issue was closed.
This change still fails on the valgrind bots: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... Syscall param pwrite64(buf) points to uninitialised byte(s) 0x717E6C3 (/build/buildd/eglibc-2.15/nptl/../sysdeps/unix/syscall-template.S:82) base::WritePlatformFile(int, long, char const*, int) (base/platform_file_posix.cc:246) disk_cache::SimpleSynchronousEntry::InitializeForCreate() (net/disk_cache/simple/simple_synchronous_entry.cc:296) disk_cache::SimpleSynchronousEntry::CreateEntry(base::FilePath const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, scoped_refptr<base::TaskRunner> const&, base::Callback<void (disk_cache::SimpleSynchronousEntry*)> const&) (net/disk_cache/simple/simple_synchronous_entry.cc:90) Address 0x408f944 is on thread 3's stack You can do targeted valgrind try runs with something like: git try linux_valgrind:net_unittests:DiskCache*
On 2013/04/12 19:41:58, Reid Kleckner wrote: > This change still fails on the valgrind bots: > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%2520Tests%2520... > > Syscall param pwrite64(buf) points to uninitialised byte(s) > 0x717E6C3 > (/build/buildd/eglibc-2.15/nptl/../sysdeps/unix/syscall-template.S:82) > base::WritePlatformFile(int, long, char const*, int) > (base/platform_file_posix.cc:246) > disk_cache::SimpleSynchronousEntry::InitializeForCreate() > (net/disk_cache/simple/simple_synchronous_entry.cc:296) > disk_cache::SimpleSynchronousEntry::CreateEntry(base::FilePath const&, > std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, > scoped_refptr<base::TaskRunner> const&, base::Callback<void > (disk_cache::SimpleSynchronousEntry*)> const&) > (net/disk_cache/simple/simple_synchronous_entry.cc:90) > Address 0x408f944 is on thread 3's stack > > You can do targeted valgrind try runs with something like: > git try linux_valgrind:net_unittests:DiskCache* Awesome! Thanks for the trace! I left out one more uninitialized read, happy to see Valgrind catches it (did not catch on my local machine, hm..). Valgrind trybot passes now with a small change analogous to what I did in here: https://codereview.chromium.org/14204002 Going with another round.
Message was sent while issue was closed.
Committed patchset #12 manually as r194173 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2703: // File renaming is flaky on Windows, disable all Simple Backend Tests there. On 2013/04/12 17:39:30, rvargas wrote: > I know Windows is not your priority, but I'm not convinced by this explanation. > Could you clarify that for me / file the appropriate bugs to track and fix > whatever is broken? It could be the problem with Windows ReplaceFile() that fails when the target file does not exist. This is fixable, but as you said: Windows is not a priority right now for the simple cache. We use the atomic rename on the index file because on Linux/Android/MacOS it is guaranteed to remain either on the old version or on the new one. Modulo OS bugs and network filesystems, of course. AFAIR There is a problem that Windows XP sometimes cannot rename a file to the target file if the latter was recently open, especially with antivirus software running on the machine. ReplaceFile() happens to see the target file as open and fails to replace it. I did not find a good place to point to as a proof of this. It is rather my suspicion that we may end up fighting flakiness for a lot longer than needed for the simple cache. I am OK to disable the simple cache backend tests on Windows by any means you prefer.
Message was sent while issue was closed.
https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13731002/diff/54001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2703: // File renaming is flaky on Windows, disable all Simple Backend Tests there. On 2013/04/15 19:13:54, pasko wrote: > On 2013/04/12 17:39:30, rvargas wrote: > > I know Windows is not your priority, but I'm not convinced by this > explanation. > > Could you clarify that for me / file the appropriate bugs to track and fix > > whatever is broken? > > It could be the problem with Windows ReplaceFile() that fails when the target > file does not exist. This is fixable, but as you said: Windows is not a priority > right now for the simple cache. > > We use the atomic rename on the index file because on Linux/Android/MacOS it is > guaranteed to remain either on the old version or on the new one. Modulo OS bugs > and network filesystems, of course. > > AFAIR There is a problem that Windows XP sometimes cannot rename a file to the > target file if the latter was recently open, especially with antivirus software > running on the machine. ReplaceFile() happens to see the target file as open and > fails to replace it. I did not find a good place to point to as a proof of this. > It is rather my suspicion that we may end up fighting flakiness for a lot longer > than needed for the simple cache. > > I am OK to disable the simple cache backend tests on Windows by any means you > prefer. I have not looked into the failures that you mention, but, especially in the context of this CL, they seem suspicious. We have chromium code (implemented for all platforms) to perform file updates. I have not been following the simple cache enough to know what are you guys using or where are you getting issues though. As for the solution, if you guys say that the simple cache cannot run on Windows yet (I doubt it can anyways, knowing that it has paths that have not been implemented) then #ifdef on the block of tests is a better solution than DISABLE_xx... and maybe do nothing from the test framework level so that we don't litter the test files with ifdefs. DISABLED should not be used for code that simply has not been written yet (unless we're talking about a few days, which doesn't seem to be the case here). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
