Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
901303002:
Make SDCH dictionaries persistent across browser restart. (Closed)
|
Created:
5 years, 10 months ago by Elly Fong-Jones Modified:
5 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, erikchen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake SDCH dictionaries persistent across browser restart.
This CL also adds a generic persistence mechanism for //net, separate
from Chrome user preferences, that loads asynchronously on startup (i.e.
doesn't block startup).
This is a reupload of rdsmith@'s existing CL at
https://codereview.chromium.org/881413003/; see that CL for earlier discussion.
BUG=374915
R=mmenke@chromium.org
R=rdsmith@chromium.org
R=bauerb@chromium.org
Committed: https://crrev.com/d0ff2559cf187088623898f0b2dd95841123cf6c
Cr-Commit-Position: refs/heads/master@{#319753}
Patch Set 1 #
Total comments: 5
Patch Set 2 : rdsmith's original version #Patch Set 3 : New version #Patch Set 4 : Fixes for bauerb #
Total comments: 31
Patch Set 5 : More fixes #
Total comments: 69
Patch Set 6 : unit testing intensifies #Patch Set 7 : More fixes #
Total comments: 15
Patch Set 8 : Fixes for rdsmith #
Total comments: 34
Patch Set 9 : Add field trial #Patch Set 10 : Bunch of nits fixed. #Patch Set 11 : Add unit tests for attempted load #
Total comments: 8
Patch Set 12 : Rework attempted_load_ logic in SdchDictionaryFetcher #Patch Set 13 : Beef up SdchDictionaryFetcher tests #
Total comments: 50
Patch Set 14 : More fixes #
Total comments: 4
Patch Set 15 : Last round #
Total comments: 3
Patch Set 16 : Rebase #Patch Set 17 : Fix cache control headers #Messages
Total messages: 59 (13 generated)
ptal? :)
Could you upload the old version as well, so it's easier to compare them? https://codereview.chromium.org/901303002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/901303002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); AFAIU the discussion on the previous CL, the consensus was to remove this check. https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.h:87: QueuedInfo(const QueuedInfo& rhs) = default; Hm... I think technically this defines the constructor inline, which you're not supposed to do for nontrivial implementations. I think the right thing here would be to just declare it here, and use default for the definition: { [...] QueuedInfo(const QueuedInfo& rhs); [...] } QueuedInfo::QueuedInfo(const QueuedInfo& rhs) = default; Or, you could could only forward-declare the struct here and then define it in the .cc file, at which point it wouldn't really matter whether the methods are inlined or not.
ptal? https://codereview.chromium.org/901303002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/901303002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); On 2015/02/06 16:40:27, Bernhard Bauer wrote: > AFAIU the discussion on the previous CL, the consensus was to remove this check. Done. https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.h:87: QueuedInfo(const QueuedInfo& rhs) = default; On 2015/02/06 16:40:27, Bernhard Bauer wrote: > Hm... I think technically this defines the constructor inline, which you're not > supposed to do for nontrivial implementations. I think the right thing here > would be to just declare it here, and use default for the definition: > > { > [...] > QueuedInfo(const QueuedInfo& rhs); > [...] > } > > QueuedInfo::QueuedInfo(const QueuedInfo& rhs) = default; > > Or, you could could only forward-declare the struct here and then define it in > the .cc file, at which point it wouldn't really matter whether the methods are > inlined or not. Done. We can only have an operator= if the GURL member is not const, so I made it not const.
Nice! LGTM, with a formatting nit and a question. https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.h:87: QueuedInfo(const QueuedInfo& rhs) = default; On 2015/02/06 18:44:09, Elly Jones wrote: > On 2015/02/06 16:40:27, Bernhard Bauer wrote: > > Hm... I think technically this defines the constructor inline, which you're > not > > supposed to do for nontrivial implementations. I think the right thing here > > would be to just declare it here, and use default for the definition: > > > > { > > [...] > > QueuedInfo(const QueuedInfo& rhs); > > [...] > > } > > > > QueuedInfo::QueuedInfo(const QueuedInfo& rhs) = default; > > > > Or, you could could only forward-declare the struct here and then define it in > > the .cc file, at which point it wouldn't really matter whether the methods are > > inlined or not. > > Done. We can only have an operator= if the GURL member is not const, so I made > it not const. Makes sense, if we allow assignment. (What does that mean for the old version? Did we silently ignore assigning the URL?) https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:213: &last_used_seconds_from_epoch); This seems to be aligned weirdly.
I'm happy with SdchOwner now...Haven't taken the time to look at the rest yet, though. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:164: bool IsAtEnd() const { return dictionary_iterator_.IsAtEnd(); } Seems weird to inline this, but not the other methods use capital camel case - suggest de-inlining it. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:168: GURL url() const { return url_; } const GURL& https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:258: net::URLRequestContext* context) While you're here, these net::'s aren't needed. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:275: InitializePersistentStore(pref_store_); "EnablePersistentStorage" is used to mean the on-disk store, so it seems weird to be calling "InitializePersistentStore" on the in-memory store. Suggest renaming it to something like "InitializePreferenceStore" https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:282: DICTIONARY_FATE_EVICT_FOR_DESTRUCTION); Hrm...Not sure "evict" is still accurate here, at least if we're using an on-disk store https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:345: base::Time never_used_stale_boundary( Think these two "boundary" times are worth a short comment before each. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:355: } Probably a bit beyond the scope of this cl, but should we even bother doing this if total_dictionary_bytes_ + dictionary_text.size() > max_total_dictionary_size_? I guess we don't keep too many dictionaries around, but this takes O(n^2) time during startup. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:444: // the following DCHECKs? This TODOs seem rather concerned. If they're not, we're introducing a crasher. Figure that out before landing? https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:452: &last_used_time); nit: Should use consistent variable names here and in the similar OrDie method. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:523: break; Wonder if we should record something here - otherwise, not sure how meaningful the error histograms are https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:545: } optional: I'm not a big fan of fallthrough behavior. Think it's easy to miss when reading code. I'd suggest something like: if (!succeeded) { DCHECK_NE(error, PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE); DCHECK_NE(...); RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED); return; } Then have a switch for recoverable errors. One difference is we'd be handling success and PREF_READ_ERROR_ACCESS_DENIED / PREF_READ_ERROR_FILE_NOT_SPECIFIED differently (Can we see success on those errors?) https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:589: external_pref_store_->RemoveObserver(this); Should we do this on failure, too? (And maybe null out the external pref store in that case, too?) https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:128: scoped_refptr<ValueMapPrefStore> in_memory_pref_store_; Should include base/memory/ref_counted.h
New patchsets have been uploaded after l-g-t-m from bauerb@chromium.org
ptal? https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:164: bool IsAtEnd() const { return dictionary_iterator_.IsAtEnd(); } On 2015/02/06 22:29:41, mmenke wrote: > Seems weird to inline this, but not the other methods use capital camel case - > suggest de-inlining it. Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:168: GURL url() const { return url_; } On 2015/02/06 22:29:41, mmenke wrote: > const GURL& Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:213: &last_used_seconds_from_epoch); On 2015/02/06 20:12:18, Bernhard Bauer wrote: > This seems to be aligned weirdly. Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:258: net::URLRequestContext* context) On 2015/02/06 22:29:41, mmenke wrote: > While you're here, these net::'s aren't needed. Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:275: InitializePersistentStore(pref_store_); On 2015/02/06 22:29:42, mmenke wrote: > "EnablePersistentStorage" is used to mean the on-disk store, so it seems weird > to be calling "InitializePersistentStore" on the in-memory store. Suggest > renaming it to something like "InitializePreferenceStore" Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:282: DICTIONARY_FATE_EVICT_FOR_DESTRUCTION); On 2015/02/06 22:29:41, mmenke wrote: > Hrm...Not sure "evict" is still accurate here, at least if we're using an > on-disk store Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:345: base::Time never_used_stale_boundary( On 2015/02/06 22:29:42, mmenke wrote: > Think these two "boundary" times are worth a short comment before each. Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:355: } On 2015/02/06 22:29:42, mmenke wrote: > Probably a bit beyond the scope of this cl, but should we even bother doing this > if total_dictionary_bytes_ + dictionary_text.size() > > max_total_dictionary_size_? > > I guess we don't keep too many dictionaries around, but this takes O(n^2) time > during startup. It does, but I think n is small. I am going to not worry about this for now. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:444: // the following DCHECKs? On 2015/02/06 22:29:41, mmenke wrote: > This TODOs seem rather concerned. If they're not, we're introducing a crasher. > Figure that out before landing? I admit that I have no idea what this TODO is referring to. The pref dictionary map is internal to this class, so its consistency is our problem. I would guess that it refers to the possibility of SdchManager sending us this event for a dictionary that SdchOwner doesn't know about, which I think should be a DCHECK-worthy condition. In any case, this TODO doesn't need to be here; either SdchManager is buggy and this DCHECK will catch that, or it's not and it won't. Removing the DCHECK would just introduce a hard crash here when we try to use the dictionary. I have removed the TODO. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:452: &last_used_time); On 2015/02/06 22:29:42, mmenke wrote: > nit: Should use consistent variable names here and in the similar OrDie method. Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:523: break; On 2015/02/06 22:29:41, mmenke wrote: > Wonder if we should record something here - otherwise, not sure how meaningful > the error histograms are They still tell us the relative frequency of the different kinds of error. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:545: } On 2015/02/06 22:29:41, mmenke wrote: > optional: I'm not a big fan of fallthrough behavior. Think it's easy to miss > when reading code. > > I'd suggest something like: > > if (!succeeded) { > DCHECK_NE(error, PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE); > DCHECK_NE(...); > RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED); > return; > } > > Then have a switch for recoverable errors. One difference is we'd be handling > success and PREF_READ_ERROR_ACCESS_DENIED / PREF_READ_ERROR_FILE_NOT_SPECIFIED > differently (Can we see success on those errors?) Done. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:589: external_pref_store_->RemoveObserver(this); On 2015/02/06 22:29:42, mmenke wrote: > Should we do this on failure, too? (And maybe null out the external pref store > in that case, too?) We do do this on the failure case, I think? https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:128: scoped_refptr<ValueMapPrefStore> in_memory_pref_store_; On 2015/02/06 22:29:42, mmenke wrote: > Should include base/memory/ref_counted.h Done.
Hope to continue reviewing this this afternoon - still have more files to look at. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:589: external_pref_store_->RemoveObserver(this); On 2015/02/11 21:23:51, Elly Jones wrote: > On 2015/02/06 22:29:42, mmenke wrote: > > Should we do this on failure, too? (And maybe null out the external pref > store > > in that case, too?) > > We do do this on the failure case, I think? Oops...You're right, we are. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:543: DCHECK(error != PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM); DCHECK_NE for both of these? https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:547: RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED); optional nitty nit: Just personal preference, but I think logging before handling an error case is a little cleaner. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:140: DCHECK(success); Suggest a helper method for these 4 lines...Or maybe just a "GetFirstLevelDict" method? Actually, do we care about anything other than the SDCH dictionary? Rather than pass that everywhere, maybe just GetSDCHDictionary? https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:140: DCHECK(success); Read my comments in the tests themselves before doing anything here...I'm not sure we should be creating all these dictionaries manually. Also, since we know the structure of the dictionary, I think we can simplify the ones we do need. i.e. "SetExpiractionTime(const GURL&, base::Time);" If we ever modify the format of the dictionary, that works better, too. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:200: success = dict2_as_value->GetAsDictionary(&dict2_as_dict); Can just use dict1_as_dict->GetDictionary(&dict2); (Or GetDictionaryWithoutPathExpansion) https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:212: success = dict3_as_value->GetAsDictionary(&dict3_as_dict); Again, can just GetDictionary...Or GetDictionaryWithoutPathExpansion. Or if don't mind using path expansion, could actually just use the top level dict, and use GetDictionary(key2 + "/" + key3); https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:656: pref_store().SetInitializationCompleted(); We should have a test where this happens after initialization. And one where it happens after destruction of the dictionary. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:675: "last_used", base::Time::Now().ToDoubleT()); I wonder about hard coding all this. Can we just create an SDCH manager with a pref store, have it load the dictionaries, flush it to disk and clean it up, and then load from that pref store? That means we don't have to worry about changes to the format, and also means we test the saving as well as the loading of the dictionary, which we aren't currently testing. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:735: } Other test suggestions: * If we load a dict and the init completes, we correctly merge the list of dictionaries. * Eviction has the right priority for loaded dictionaries (A loaded used dictionary that's, say, 25 hours old can be evicted, as can a loaded unused one that's 2 hours old, but a loaded used one that's 12 hours old is not - could have one test with all those dictionaries). https://codereview.chromium.org/901303002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/901303002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:56400: + <int value="9" label="ADD_PERSISTENCE_TRIGGERRED"/> DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION = 10
https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:107: ResetRequest(); I think this is ugly...I have some other suggestions about what to do in this case elsewhere, but I suggest for this block: int result = request->status().error(); if (result == OK && response_headers) { ... if (validation_type != VALIDATION_NONE) { // There's a cache entry, but it's expired. Treat it as a failed request. result = ERR_FAILED; } } DoLoop(result); The advantage here is that you go through the exact same flow as for requests that are actually failed, which I think reduces complexity a bit. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:155: DCHECK(CalledOnValidThread()); Include base/logging.h https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:195: if (!in_loop_) I think this in_loop_ check is pretty ugly. Suggest getting rid of it and moving the next_state_ = BLAH out of the method. Then at both callsites, set the next state, and only enter the DoLoop where needed. Alternatively, could make this a new state (STATE_RESET_REQUEST?). Could put it at the start of STATE_SEND_REQUEST, and everything would work, but that seems icky. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:249: : 0)); optional: This is one cases where I think using a local is clearer. i.e. int load_flags = blah; if (blah) load_flags |= blah; current_request_->SetLoadFlags(blah); https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:252: current_callback_ = fetch_queue_.front().callback; optional: Might be simplest just to get rid of current_callback_, and have the invariant that the current fetch is for the object at the front of the queue. Either way, should document whichever is the case in the header. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:48: SdchDictionaryFetcher(URLRequestContext* context); explicit https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:78: struct QueuedInfo { QueuedFetchInfo? And maybe a quick comment... https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:81: OnDictionaryFetchedCallback callback; Member variables should go after methods. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:88: QueuedInfo& operator=(const QueuedInfo& rhs); Hrm.... Per style guide, constructor and destructor should go first... Doesn't say anything about operator=... Does seem to make sense with the constructors, though, so I'm fine either way, jsut thought I'd mention it. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:114: // |fetch_queue_[n].second| indicated whether the download should occur second -> download_only_from_cache https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:42: last_load_flags_seen_(0), optional: Suggest just grabbing them here. We don't support changing load flags between when a URLRequestJob is created and when it's started, anyways. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:174: last_load_flags_seen_(0), optional: Suggest LOAD_NONE here. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:181: 0)), Think keeping this around is a little ugly - if we overrode it anywhere, that would be fine, but since we don't, suggest just making default_callback() create it and rename the method to [Get]DefaultCallback(). https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:186: response_info_to_return_.response_time = base::Time::Now(); This seems a little unexpected...Should probably mention it where response_info_to_return_ is declared. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:238: } Need to document this. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:243: } Suggest not doing this in this CL - doesn't seem to save anything, and this file is already changed enough. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:248: const SdchDictionaryFetcher::OnDictionaryFetchedCallback& default_callback() { + const? https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:267: int last_tag_; These new variables need documentation, particularly "tag". https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:295: TEST_F(SdchDictionaryFetcherTest, SpecializedCallback) { This test doesn't seem to get us anything...How would it call us without a callback? If you want to do two lookups at once, and make sure that the first callback isn't "sticky", I'm fine with that. Otherwise, don't think we get anything from the test. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:385: response_info_to_return()->headers = new HttpResponseHeaders(""); BUG: new HttpResponseHeaders("\0") Give that's pretty hideous, could just use: std::string headers = "blah" "new HttpResponseHeaders(HttpUtil::AssembleRawHeaders( blah.c_str(), blah.size()); We should probably have a utility method to do that for tests (Or maybe even for production code), but looks like we don't. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:413: ASSERT_EQ(0u, additions.size()); nit: EXPECT_EQ
There are still two unresolved comments, but I replied pointing out what they are. PTAL anyway? :) https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:543: DCHECK(error != PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM); On 2015/02/12 16:51:32, mmenke wrote: > DCHECK_NE for both of these? Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:547: RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED); On 2015/02/12 16:51:32, mmenke wrote: > optional nitty nit: Just personal preference, but I think logging before > handling an error case is a little cleaner. Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:140: DCHECK(success); On 2015/02/12 16:51:33, mmenke wrote: > Read my comments in the tests themselves before doing anything here...I'm not > sure we should be creating all these dictionaries manually. Also, since we know > the structure of the dictionary, I think we can simplify the ones we do need. > i.e. "SetExpiractionTime(const GURL&, base::Time);" If we ever modify the > format of the dictionary, that works better, too. Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:140: DCHECK(success); On 2015/02/12 16:51:33, mmenke wrote: > Suggest a helper method for these 4 lines...Or maybe just a "GetFirstLevelDict" > method? Actually, do we care about anything other than the SDCH dictionary? > Rather than pass that everywhere, maybe just GetSDCHDictionary? Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:200: success = dict2_as_value->GetAsDictionary(&dict2_as_dict); On 2015/02/12 16:51:32, mmenke wrote: > Can just use dict1_as_dict->GetDictionary(&dict2); (Or > GetDictionaryWithoutPathExpansion) Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:212: success = dict3_as_value->GetAsDictionary(&dict3_as_dict); On 2015/02/12 16:51:33, mmenke wrote: > Again, can just GetDictionary...Or GetDictionaryWithoutPathExpansion. Or if > don't mind using path expansion, could actually just use the top level dict, and > use GetDictionary(key2 + "/" + key3); Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:656: pref_store().SetInitializationCompleted(); On 2015/02/12 16:51:33, mmenke wrote: > We should have a test where this happens after initialization. And one where it > happens after destruction of the dictionary. Where what happens after initialization? Modification of the pref store? I think we currently deliberately don't listen for that (?). https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:675: "last_used", base::Time::Now().ToDoubleT()); On 2015/02/12 16:51:32, mmenke wrote: > I wonder about hard coding all this. Can we just create an SDCH manager with a > pref store, have it load the dictionaries, flush it to disk and clean it up, and > then load from that pref store? > > That means we don't have to worry about changes to the format, and also means we > test the saving as well as the loading of the dictionary, which we aren't > currently testing. Done. https://codereview.chromium.org/901303002/diff/80001/net/sdch/sdch_owner_unit... net/sdch/sdch_owner_unittest.cc:735: } On 2015/02/12 16:51:33, mmenke wrote: > Other test suggestions: > > * If we load a dict and the init completes, we correctly merge the list of > dictionaries. Done. > * Eviction has the right priority for loaded dictionaries (A loaded used > dictionary that's, say, 25 hours old can be evicted, as can a loaded unused one > that's 2 hours old, but a loaded used one that's 12 hours old is not - could > have one test with all those dictionaries). I'll need to dig a little more to understand how to do this. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:107: ResetRequest(); On 2015/02/12 20:40:35, mmenke wrote: > I think this is ugly...I have some other suggestions about what to do in this > case elsewhere, but I suggest for this block: > > int result = request->status().error(); > if (result == OK && response_headers) { > ... > if (validation_type != VALIDATION_NONE) { > // There's a cache entry, but it's expired. Treat it as a failed request. > result = ERR_FAILED; > } > } > > DoLoop(result); > > > The advantage here is that you go through the exact same flow as for requests > that are actually failed, which I think reduces complexity a bit. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:155: DCHECK(CalledOnValidThread()); On 2015/02/12 20:40:35, mmenke wrote: > Include base/logging.h Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:195: if (!in_loop_) On 2015/02/12 20:40:35, mmenke wrote: > I think this in_loop_ check is pretty ugly. > > Suggest getting rid of it and moving the next_state_ = BLAH out of the method. > > Then at both callsites, set the next state, and only enter the DoLoop where > needed. > > Alternatively, could make this a new state (STATE_RESET_REQUEST?). Could put it > at the start of STATE_SEND_REQUEST, and everything would work, but that seems > icky. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:249: : 0)); On 2015/02/12 20:40:35, mmenke wrote: > optional: This is one cases where I think using a local is clearer. i.e. > > int load_flags = blah; > if (blah) > load_flags |= blah; > current_request_->SetLoadFlags(blah); Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.cc:252: current_callback_ = fetch_queue_.front().callback; On 2015/02/12 20:40:35, mmenke wrote: > optional: Might be simplest just to get rid of current_callback_, and have the > invariant that the current fetch is for the object at the front of the queue. > Either way, should document whichever is the case in the header. Unfortunately we pull it out of the queue right after this :(. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:48: SdchDictionaryFetcher(URLRequestContext* context); On 2015/02/12 20:40:36, mmenke wrote: > explicit Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:78: struct QueuedInfo { On 2015/02/12 20:40:36, mmenke wrote: > QueuedFetchInfo? And maybe a quick comment... Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:81: OnDictionaryFetchedCallback callback; On 2015/02/12 20:40:36, mmenke wrote: > Member variables should go after methods. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:88: QueuedInfo& operator=(const QueuedInfo& rhs); On 2015/02/12 20:40:36, mmenke wrote: > Hrm.... Per style guide, constructor and destructor should go first... Doesn't > say anything about operator=... Does seem to make sense with the constructors, > though, so I'm fine either way, jsut thought I'd mention it. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher.h:114: // |fetch_queue_[n].second| indicated whether the download should occur On 2015/02/12 20:40:36, mmenke wrote: > second -> download_only_from_cache Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:42: last_load_flags_seen_(0), On 2015/02/12 20:40:36, mmenke wrote: > optional: Suggest just grabbing them here. We don't support changing load > flags between when a URLRequestJob is created and when it's started, anyways. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:174: last_load_flags_seen_(0), On 2015/02/12 20:40:36, mmenke wrote: > optional: Suggest LOAD_NONE here. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:181: 0)), On 2015/02/12 20:40:36, mmenke wrote: > Think keeping this around is a little ugly - if we overrode it anywhere, that > would be fine, but since we don't, suggest just making default_callback() create > it and rename the method to [Get]DefaultCallback(). Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:186: response_info_to_return_.response_time = base::Time::Now(); On 2015/02/12 20:40:36, mmenke wrote: > This seems a little unexpected...Should probably mention it where > response_info_to_return_ is declared. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:238: } On 2015/02/12 20:40:36, mmenke wrote: > Need to document this. I have no idea what it is :( https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:243: } On 2015/02/12 20:40:36, mmenke wrote: > Suggest not doing this in this CL - doesn't seem to save anything, and this file > is already changed enough. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:248: const SdchDictionaryFetcher::OnDictionaryFetchedCallback& default_callback() { On 2015/02/12 20:40:36, mmenke wrote: > + const? Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:267: int last_tag_; On 2015/02/12 20:40:36, mmenke wrote: > These new variables need documentation, particularly "tag". Similarly, I have no idea what these are. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:295: TEST_F(SdchDictionaryFetcherTest, SpecializedCallback) { On 2015/02/12 20:40:36, mmenke wrote: > This test doesn't seem to get us anything...How would it call us without a > callback? > > If you want to do two lookups at once, and make sure that the first callback > isn't "sticky", I'm fine with that. Otherwise, don't think we get anything from > the test. Deleted. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:385: response_info_to_return()->headers = new HttpResponseHeaders(""); On 2015/02/12 20:40:36, mmenke wrote: > BUG: new HttpResponseHeaders("\0") > > Give that's pretty hideous, could just use: > > std::string headers = "blah" > "new HttpResponseHeaders(HttpUtil::AssembleRawHeaders( > blah.c_str(), blah.size()); > > We should probably have a utility method to do that for tests (Or maybe even for > production code), but looks like we don't. Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:413: ASSERT_EQ(0u, additions.size()); On 2015/02/12 20:40:36, mmenke wrote: > nit: EXPECT_EQ Done. https://codereview.chromium.org/901303002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/901303002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:56400: + <int value="9" label="ADD_PERSISTENCE_TRIGGERRED"/> On 2015/02/12 16:51:33, mmenke wrote: > DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION = 10 Done.
It's a very weird feeling reviewing one's own code after it's been taken over by someone else and you've been away from it for a week. My apologies in advance, Elly, for places in which I'm asking you to fix my mistakes and glitches (and there are several). But this is looking good to me. https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:444: // the following DCHECKs? On 2015/02/11 21:23:51, Elly Jones wrote: > On 2015/02/06 22:29:41, mmenke wrote: > > This TODOs seem rather concerned. If they're not, we're introducing a > crasher. > > Figure that out before landing? > > I admit that I have no idea what this TODO is referring to. The pref dictionary > map is internal to this class, so its consistency is our problem. I would guess > that it refers to the possibility of SdchManager sending us this event for a > dictionary that SdchOwner doesn't know about, which I think should be a > DCHECK-worthy condition. In any case, this TODO doesn't need to be here; either > SdchManager is buggy and this DCHECK will catch that, or it's not and it won't. > Removing the DCHECK would just introduce a hard crash here when we try to use > the dictionary. > > I have removed the TODO. +1. This was in the way of a note to myself to just confirm what you've said above, which I believe is all true. The DCHECK is asserting that the manager and the owner are in sync with regard to what dictionaries are currently active, and if it trips, that's a bug in the code (either manager or owner). https://codereview.chromium.org/901303002/diff/60001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:523: break; On 2015/02/11 21:23:51, Elly Jones wrote: > On 2015/02/06 22:29:41, mmenke wrote: > > Wonder if we should record something here - otherwise, not sure how meaningful > > the error histograms are > > They still tell us the relative frequency of the different kinds of error. +1, sorta to each point :-}. I think these histograms are useful as written as they tell us the relative error frequencies, and that information will be easier to get visually if we don't get the success case. It doesn't give us the total number of times we successfully loaded from disk (to compare against the error cases) and that is a lack--it doesn't allow us to answer the basic question of "How often is this thing working?". If we want that, we could probably get that number from the total number of browser restarts, which I gotta believe is available somewhere in UMA. But that's not very convenient. I guess that ends me up with leaning towards including the success case. But I'm willing to go along with whatever comes out of the conversation between the two of you. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:238: } On 2015/02/13 23:35:01, Elly Jones wrote: > On 2015/02/12 20:40:36, mmenke wrote: > > Need to document this. > > I have no idea what it is :( It's the HttpResponseInfo that'll be returned by the URLRequestJob. It's hacky, but it lets the test class specify what it wants the response info to look like coming back up out of request processing. (Let me know if this isn't clear.) https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:267: int last_tag_; On 2015/02/13 23:35:02, Elly Jones wrote: > On 2015/02/12 20:40:36, mmenke wrote: > > These new variables need documentation, particularly "tag". > > Similarly, I have no idea what these are. last_load_flags_seen is the last load flags that the URLRequestJob saw; it's passed up on jobs destruction. last_tag_ is a way to distinguish callbacks and make sure that we are successfully varying callbacks between different calls to fetch dictionaries; see SpecializedCallback below. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:295: TEST_F(SdchDictionaryFetcherTest, SpecializedCallback) { On 2015/02/12 20:40:36, mmenke wrote: > This test doesn't seem to get us anything...How would it call us without a > callback? > > If you want to do two lookups at once, and make sure that the first callback > isn't "sticky", I'm fine with that. Otherwise, don't think we get anything from > the test. This test wasn't to test whether we were called without a callback--it was to test to make sure we were called with the callback specified for *this* schedule call rather than some other one. Now having said that, given that the test is run in isolation, there's really no way there could have been leakage, so I'm good with deleting it. But that means you should delete the tag mechanism as well-this was the only test that used it. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:96: void RecordDictionaryUnload(int use_count) { nit: Why the duplication with the above? I'd think that it would be cleaner to either use a single function (i.e. RecordDictionaryEviction()) or use three separate functions for the three separate fates that RecordDictionaryEviction() currently allows. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:584: // TODO(rdsmith): Implement versioning. This CL actually implements probably the best versioning scheme: If versions don't match, drop the data. Given that, could you remove this line? https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:47: // actual mechanisms by which the WriteablePrefStore are persisted nit: WriteablePrefStore -> PersistentPrefStore (almost certainly my glitch). https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:110: // Returns false if the persisted_info has a bad format. nit, suggestion: Comment should describe what happens WRT load scheduling if persisted_info has a bad format? https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:131: // it is only set while the external pref store is being observed. nit: Capitalize "it". Alternatively, delete this line; it seems redundant with the next line. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner_uni... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:618: class SdchOwnerPersistenceTest : public ::testing::Test /* SdchOwnerTest */ { nit: I find this comment syntax confusing. Should this be a subclass of SdchOwnerTest or not? https://codereview.chromium.org/901303002/diff/120001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/120001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:54: const OnDictionaryFetchedCallback& callback); nit, confusion: Bad indentation? But if 'git cl format' produced this, please ignore this comment.
https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:295: TEST_F(SdchDictionaryFetcherTest, SpecializedCallback) { On 2015/02/16 01:10:04, rdsmith wrote: > On 2015/02/12 20:40:36, mmenke wrote: > > This test doesn't seem to get us anything...How would it call us without a > > callback? > > > > If you want to do two lookups at once, and make sure that the first callback > > isn't "sticky", I'm fine with that. Otherwise, don't think we get anything > from > > the test. > > This test wasn't to test whether we were called without a callback--it was to > test to make sure we were called with the callback specified for *this* schedule > call rather than some other one. Now having said that, given that the test is > run in isolation, there's really no way there could have been leakage, so I'm > good with deleting it. But that means you should delete the tag mechanism as > well-this was the only test that used it. It only has one callback to call...It literally has nothing else to call - no delegate, not other callback passed in to it. I don't see how it's possible to get anything other than kExtraDataTag unless it starts calling into random memory locations. If we were testing two callbacks with different tags, particularly if they were going on at once, I could see some value in the test, but with just one tag, I maintain it doesn't get us anything.
https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:295: TEST_F(SdchDictionaryFetcherTest, SpecializedCallback) { On 2015/02/17 16:43:45, mmenke wrote: > On 2015/02/16 01:10:04, rdsmith wrote: > > On 2015/02/12 20:40:36, mmenke wrote: > > > This test doesn't seem to get us anything...How would it call us without a > > > callback? > > > > > > If you want to do two lookups at once, and make sure that the first callback > > > isn't "sticky", I'm fine with that. Otherwise, don't think we get anything > > from > > > the test. > > > > This test wasn't to test whether we were called without a callback--it was to > > test to make sure we were called with the callback specified for *this* > schedule > > call rather than some other one. Now having said that, given that the test is > > run in isolation, there's really no way there could have been leakage, so I'm > > good with deleting it. But that means you should delete the tag mechanism as > > well-this was the only test that used it. > > It only has one callback to call...It literally has nothing else to call - no > delegate, not other callback passed in to it. I don't see how it's possible to > get anything other than kExtraDataTag unless it starts calling into random > memory locations. > > If we were testing two callbacks with different tags, particularly if they were > going on at once, I could see some value in the test, but with just one tag, I > maintain it doesn't get us anything. Sorry, I wasn't clear: In the process of writing out the explanation, I came to agree with you. So I was accepting test deletion, but saying the tag mechanism should be deleted as well.
ptal? https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:238: } On 2015/02/16 01:10:04, rdsmith wrote: > On 2015/02/13 23:35:01, Elly Jones wrote: > > On 2015/02/12 20:40:36, mmenke wrote: > > > Need to document this. > > > > I have no idea what it is :( > > It's the HttpResponseInfo that'll be returned by the URLRequestJob. It's hacky, > but it lets the test class specify what it wants the response info to look like > coming back up out of request processing. (Let me know if this isn't clear.) Done. https://codereview.chromium.org/901303002/diff/80001/net/url_request/sdch_dic... net/url_request/sdch_dictionary_fetcher_unittest.cc:267: int last_tag_; On 2015/02/16 01:10:04, rdsmith wrote: > On 2015/02/13 23:35:02, Elly Jones wrote: > > On 2015/02/12 20:40:36, mmenke wrote: > > > These new variables need documentation, particularly "tag". > > > > Similarly, I have no idea what these are. > > last_load_flags_seen is the last load flags that the URLRequestJob saw; it's > passed up on jobs destruction. last_tag_ is a way to distinguish callbacks and > make sure that we are successfully varying callbacks between different calls to > fetch dictionaries; see SpecializedCallback below. Done. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:96: void RecordDictionaryUnload(int use_count) { On 2015/02/16 01:10:05, rdsmith wrote: > nit: Why the duplication with the above? I'd think that it would be cleaner to > either use a single function (i.e. RecordDictionaryEviction()) or use three > separate functions for the three separate fates that RecordDictionaryEviction() > currently allows. Because I thought of Unloads as distinct from Eviction, basically, since the dictionary isn't evicted - it's still present on disk. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:584: // TODO(rdsmith): Implement versioning. On 2015/02/16 01:10:05, rdsmith wrote: > This CL actually implements probably the best versioning scheme: If versions > don't match, drop the data. Given that, could you remove this line? Done. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:47: // actual mechanisms by which the WriteablePrefStore are persisted On 2015/02/16 01:10:05, rdsmith wrote: > nit: WriteablePrefStore -> PersistentPrefStore (almost certainly my glitch). Done. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:110: // Returns false if the persisted_info has a bad format. On 2015/02/16 01:10:05, rdsmith wrote: > nit, suggestion: Comment should describe what happens WRT load scheduling if > persisted_info has a bad format? Done. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:131: // it is only set while the external pref store is being observed. On 2015/02/16 01:10:05, rdsmith wrote: > nit: Capitalize "it". Alternatively, delete this line; it seems redundant with > the next line. Done. https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner_uni... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:618: class SdchOwnerPersistenceTest : public ::testing::Test /* SdchOwnerTest */ { On 2015/02/16 01:10:05, rdsmith wrote: > nit: I find this comment syntax confusing. Should this be a subclass of > SdchOwnerTest or not? The comment was saying that it notionally was, but it no longer is. I deleted it. https://codereview.chromium.org/901303002/diff/120001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/120001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:54: const OnDictionaryFetchedCallback& callback); On 2015/02/16 01:10:05, rdsmith wrote: > nit, confusion: Bad indentation? But if 'git cl format' produced this, please > ignore this comment. Done.
https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:96: void RecordDictionaryUnload(int use_count) { On 2015/02/17 20:35:29, Elly Jones wrote: > On 2015/02/16 01:10:05, rdsmith wrote: > > nit: Why the duplication with the above? I'd think that it would be cleaner > to > > either use a single function (i.e. RecordDictionaryEviction()) or use three > > separate functions for the three separate fates that > RecordDictionaryEviction() > > currently allows. > > Because I thought of Unloads as distinct from Eviction, basically, since the > dictionary isn't evicted - it's still present on disk. Ah, good catch. That makes sense. I'm still not comfortable with the code duplication for so few lines, so I'd suggest you just hoist everything up a level and nuke these two routines, but up to you. One problem this raises, however, is that if we're persisting the dictionaries, we're also persisting the use_count, so I think this is going to mess with the DictionaryUseCount histogram. The simplest way to deal with this is to not record DictionaryUseCount on unload, but that means that for some contexts it may never get recorded, which I'm not sure how to handle. This is especially true since the likely failure mode for persisted dictionaries is that after some number of browser restarts we'll simply fail to load them from cache because of expiration, which will mean we won't record their final demise in DictionaryFate either. I'm inclined to think the right way to deal with this is to record the use count on unload, but *not* persist it, so we get use count / dictionary / browser session, but I'm open to other options. And you might want to consider implementing the todo somewhere about invoking the fetcher callback on failure, and having that callback record the fate of the dictionary on persistence read-in failure (up to you, but now that I've realized that dictionaries can die without our noticing, it's nagging at me).
Apologies for the late comments; I made a note to come back to this stuff, and lost the note. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:167: // is iterating over does not export begin()/end() methods. Could you also add a note to this comment that this iterator should only be used on a sanitized pref-store, i.e. not on the just-loaded-from-disk prefstore, as it assumes a specific format for the pref store? https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:84: int GetDictionaryCount() const; Hmmm. This is only used for testing, and the information should be more naturally available from the Manager. Willing to add a ForTesting to the end? https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:87: bool HasDictionaryFrom(const GURL& url) const; Same here (only used for testing, information is more naturally IMO gotten from the manager, add a "ForTesting"?)
ellyjones@chromium.org changed reviewers: + asvitkine@chromium.org
histograms LGTM However, unrelated to the histograms review, I think M42 is aimed at being a performance release - and one of the things we're focusing is start up performance. Do you have a good sense of the effect on start up performance of this change, given it's loading an additional pref store (albeit asynchronously) during the course of start up? If not, perhaps it would make sense to run this as an experiment to get the data? (Happy to guide you how to do that if you'd like.) +gab@ who's leading the start up perf effort for M42. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:24: bool GetDictionaryForURL(TestingPrefStore* store, const GURL& url, Nit: 1 param per line when params wrap. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:217: }; Nit: DISALLOW_ https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:77: }; Nit: DISALLOW()?
ellyjones@chromium.org changed reviewers: + thakis@chromium.org
Also worth mentioning that this also adds loading a couple cached SDCH dictionaries into memory (Also asynchronously) after the perf store. I think I've convinced Randy that the dictionary loading isn't something we want to be doing long term (We do want to keep the pref store, though - there's a lot of metadata the network stack keeps about servers, and I'd like to see us move it into an easily accessible location, so we can hook it up when using the network stack as a library outside of Chrome). On 2015/02/19 16:25:32, Alexei Svitkine wrote: > histograms LGTM > > However, unrelated to the histograms review, I think M42 is aimed at being a > performance release - and one of the things we're focusing is start up > performance. > > Do you have a good sense of the effect on start up performance of this change, > given it's loading an additional pref store (albeit asynchronously) during the > course of start up? > > If not, perhaps it would make sense to run this as an experiment to get the > data? (Happy to guide you how to do that if you'd like.) > > +gab@ who's leading the start up perf effort for M42. > > https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... > File net/sdch/sdch_owner_unittest.cc (right): > > https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... > net/sdch/sdch_owner_unittest.cc:24: bool GetDictionaryForURL(TestingPrefStore* > store, const GURL& url, > Nit: 1 param per line when params wrap. > > https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... > net/sdch/sdch_owner_unittest.cc:217: }; > Nit: DISALLOW_ > > https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... > File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): > > https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... > net/url_request/sdch_dictionary_fetcher_unittest.cc:77: }; > Nit: DISALLOW()?
Has https://code.google.com/p/chromium/issues/detail?id=374915#c10 been addressed somehow? Or is that just baloney? I'm skeptical of us doing more SDCH stuff given that the underlying SDCH library (open-vcdiff) is super dead and nobody working on SDCH seems to be interested in owning it. There's a bunch of stuff in that library I'd like to have fixed (I even wrote patches that got ignored), and I was kind of hoping we could kill SDCH at some point. (having said that, the chrome/common changes definitely lgtm, code-wise.)
On 2015/02/19 16:34:23, Nico wrote: > Has https://code.google.com/p/chromium/issues/detail?id=374915#c10 been > addressed somehow? Or is that just baloney? No, I took that comment very seriously, and there's an eviction scheme in for SDCH dictionaries now (seej https://codereview.chromium.org/841883002). Eviction happens either under memory pressure or when a new dictionary is loaded and an old one hasn't been touched for more than 24 hours. > I'm skeptical of us doing more SDCH stuff given that the underlying SDCH library > (open-vcdiff) is super dead and nobody working on SDCH seems to be interested in > owning it. There's a bunch of stuff in that library I'd like to have fixed (I > even wrote patches that got ignored), and I was kind of hoping we could kill > SDCH at some point. Thanks, that's really useful input. I think my reaction is that if we move on to do delta-encoding, we'll need to make a decision on what encoding methods (e.g. vcdiff) we support and if we decide to support it, we'll have to adopt that library. If we don't move on to delta-encoding, at that point we should revisit the question of whether SDCH is worth it (i.e. whether we should adopt vcdiff). But I think the go/no-go on delta-encoding is the right time to incorporate the vcdiff maintenance cost/question. > (having said that, the chrome/common changes definitely lgtm, code-wise.) Thank you :-}.
Cool. 387883 wasn't marked fixed when I asked, hence the question. Thanks for updating that bug! On Feb 19, 2015 8:43 AM, <rdsmith@chromium.org> wrote: > On 2015/02/19 16:34:23, Nico wrote: > >> Has https://code.google.com/p/chromium/issues/detail?id=374915#c10 been >> addressed somehow? Or is that just baloney? >> > > No, I took that comment very seriously, and there's an eviction scheme in > for > SDCH dictionaries now (seej https://codereview.chromium.org/841883002). > Eviction happens either under memory pressure or when a new dictionary is > loaded > and an old one hasn't been touched for more than 24 hours. > > I'm skeptical of us doing more SDCH stuff given that the underlying SDCH >> > library > >> (open-vcdiff) is super dead and nobody working on SDCH seems to be >> interested >> > in > >> owning it. There's a bunch of stuff in that library I'd like to have >> fixed (I >> even wrote patches that got ignored), and I was kind of hoping we could >> kill >> SDCH at some point. >> > > Thanks, that's really useful input. I think my reaction is that if we > move on > to do delta-encoding, we'll need to make a decision on what encoding > methods > (e.g. vcdiff) we support and if we decide to support it, we'll have to > adopt > that library. If we don't move on to delta-encoding, at that point we > should > revisit the question of whether SDCH is worth it (i.e. whether we should > adopt > vcdiff). But I think the go/no-go <https://goto.google.com/no-go> on > delta-encoding is the right time to > incorporate the vcdiff maintenance cost/question. > > (having said that, the chrome/common changes definitely lgtm, code-wise.) >> > > Thank you :-}. > > > > https://codereview.chromium.org/901303002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:84: int GetDictionaryCount() const; On 2015/02/17 20:57:42, rdsmith wrote: > Hmmm. This is only used for testing, and the information should be more > naturally available from the Manager. Willing to add a ForTesting to the end? +1 https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:87: bool HasDictionaryFrom(const GURL& url) const; On 2015/02/17 20:57:42, rdsmith wrote: > Same here (only used for testing, information is more naturally IMO gotten from > the manager, add a "ForTesting"?) +1 https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:12: Remove blank line https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); This should have an explanatory comment. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); I don't think we have any tests for this. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); Not relevant to this CL, but I'm rather skeptical of |attempted_load_|...Once it's true, it can stay true until Chrome is restarted, which seems rather weird. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); Also, if we're still loading something from the cache on startup, and then we see a real request try to use it, calling Schedule, the metadata from the new fetch will completely overwrite the same info loaded from the pref file, if the cache load succeeds (use count, etc). That seems kinda weird. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:194: next_state_ = STATE_SEND_REQUEST; I thought we were going to remove the next_state_ thing from here? Assuming clearing the current request means we're ready to start the next one makes me very nervous. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:77: }; On 2015/02/19 16:25:32, Alexei Svitkine wrote: > Nit: DISALLOW()? And include the header for it.
gab@chromium.org changed reviewers: + gab@chromium.org
From a performance P.O.V. I'm mostly okay with this given the read is asynchronous and off the BlockingPool (which is what I'd like more and more things to be over time), three questions though: 1) Could you possibly delay this further (i.e. although this is asynchronous, IO contention is high on startup and should be avoided at all cost -- we already do way too much of it. Could you delay even kicking off this async read until further down the startup line -- we unfortunately don't have much better than PostDelayedTask with a random number for this at the moment though... is kicking off the read lazily an option for you? If so, how early would the lazy task kick off in practice?). 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON which results in much bigger payloads than would be required to store the same data in a binary format typically (sure this is a Chrome-wide issue with existing stores, but it would be nice to not add more JSON stores as we evaluate whether to replace existing ones). 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf tests would somewhat trigger this code, but not in a significant way as it won't have a large amount of dictionaries stored on disk. Please make sure that the dirty profile used by startup.warm/cold.dirty tests is forged with a large database for this storage type (contact erikchen@ for information on how to do this). Lastly, if you have trouble answering the above questions, please land this as an A/B Finch experiment (e.g., controlling whether or not to create/hook the |network_json_store_| in ProfileImplIOData::InitializeInternal) which will allow you to see confidence intervals on key startup metrics to determine whether this affects startup. (oh and lastly, I assume that these databases will grow overtime and thus that the problem, if there is one, may be invisible at first and grow over time so we should run such an experiment for a while before deriving any conclusion). Feel free to loop me in on any further performance discussions for this work. Thanks! Gab
On 2015/02/19 20:26:39, gab wrote: > From a performance P.O.V. I'm mostly okay with this given the read is > asynchronous and off the BlockingPool (which is what I'd like more and more > things to be over time), three questions though: > > 1) Could you possibly delay this further (i.e. although this is asynchronous, IO > contention is high on startup and should be avoided at all cost -- we already do > way too much of it. Could you delay even kicking off this async read until > further down the startup line -- we unfortunately don't have much better than > PostDelayedTask with a random number for this at the moment though... is kicking > off the read lazily an option for you? If so, how early would the lazy task kick > off in practice?). I think it's fine to kick the can a bit further down the startup timeline; want to suggest a random number? I can imagine usage patterns on mobile resulting in this being less than ideal, but I have no interest in making that argument without data. (Side note: I'd love to have a more general way to address this problem; dispatch when load under some value? But that's not for this CL.) > 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON which > results in much bigger payloads than would be required to store the same data in > a binary format typically (sure this is a Chrome-wide issue with existing > stores, but it would be nice to not add more JSON stores as we evaluate whether > to replace existing ones). I picked JSON pretty much for simplicity of programming. I have no long-term objection to switching to a different format (I'd like it to be general enough to support any reasonable network stack persistence needs, but that still leaves a lot of room). But I'd really like to get this into M42, and I can't imagine we could turn around a new persistent store in a day. Is that switch something you'd be ok with doing in a separate CL? > 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf tests > would somewhat trigger this code, but not in a significant way as it won't have > a large amount of dictionaries stored on disk. Please make sure that the dirty > profile used by startup.warm/cold.dirty tests is forged with a large database > for this storage type (contact erikchen@ for information on how to do this). So in practical terms in the field the limit on dictionary size is going to be 2 dictionaries of 500k each. That's a hard limit on mobile, and a current likely limit on desktop because Google's the only server using SDCH at the moment. I don't know if that changes your perspective on the perf waterfall at all, but it's at least useful background information. > Lastly, if you have trouble answering the above questions, please land this as > an A/B Finch experiment (e.g., controlling whether or not to create/hook the > |network_json_store_| in ProfileImplIOData::InitializeInternal) which will allow > you to see confidence intervals on key startup metrics to determine whether this > affects startup. (oh and lastly, I assume that these databases will grow > overtime and thus that the problem, if there is one, may be invisible at first > and grow over time so we should run such an experiment for a while before > deriving any conclusion). I actually don't think the DBs will grow over time, at least in the current usage configuration. The current environment will only result in two dictionaries, and nothing besides SDCH is currently using the pref store, so the asynchronous pref read-in will be under 1K of text (SWAG), and the dictionary read-in will be O(1M). And it'll probably get to that level pretty quickly :-}. > Feel free to loop me in on any further performance discussions for this work. > > Thanks! > Gab
On 2015/02/19 20:39:33, rdsmith wrote: > On 2015/02/19 20:26:39, gab wrote: > > From a performance P.O.V. I'm mostly okay with this given the read is > > asynchronous and off the BlockingPool (which is what I'd like more and more > > things to be over time), three questions though: > > > > 1) Could you possibly delay this further (i.e. although this is asynchronous, > IO > > contention is high on startup and should be avoided at all cost -- we already > do > > way too much of it. Could you delay even kicking off this async read until > > further down the startup line -- we unfortunately don't have much better than > > PostDelayedTask with a random number for this at the moment though... is > kicking > > off the read lazily an option for you? If so, how early would the lazy task > kick > > off in practice?). > > I think it's fine to kick the can a bit further down the startup timeline; want > to suggest a random number? I can imagine usage patterns on mobile resulting in > this being less than ideal, but I have no interest in making that argument > without data. > > (Side note: I'd love to have a more general way to address this problem; > dispatch when load under some value? But that's not for this CL.) > > > 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON which > > results in much bigger payloads than would be required to store the same data > in > > a binary format typically (sure this is a Chrome-wide issue with existing > > stores, but it would be nice to not add more JSON stores as we evaluate > whether > > to replace existing ones). > > I picked JSON pretty much for simplicity of programming. I have no long-term > objection to switching to a different format (I'd like it to be general enough > to support any reasonable network stack persistence needs, but that still leaves > a lot of room). But I'd really like to get this into M42, and I can't imagine > we could turn around a new persistent store in a day. Is that switch something > you'd be ok with doing in a separate CL? > > > 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf tests > > would somewhat trigger this code, but not in a significant way as it won't > have > > a large amount of dictionaries stored on disk. Please make sure that the dirty > > profile used by startup.warm/cold.dirty tests is forged with a large database > > for this storage type (contact erikchen@ for information on how to do this). > > So in practical terms in the field the limit on dictionary size is going to be 2 > dictionaries of 500k each. That's a hard limit on mobile, and a current likely > limit on desktop because Google's the only server using SDCH at the moment. I > don't know if that changes your perspective on the perf waterfall at all, but > it's at least useful background information. > > > Lastly, if you have trouble answering the above questions, please land this as > > an A/B Finch experiment (e.g., controlling whether or not to create/hook the > > |network_json_store_| in ProfileImplIOData::InitializeInternal) which will > allow > > you to see confidence intervals on key startup metrics to determine whether > this > > affects startup. (oh and lastly, I assume that these databases will grow > > overtime and thus that the problem, if there is one, may be invisible at first > > and grow over time so we should run such an experiment for a while before > > deriving any conclusion). > > I actually don't think the DBs will grow over time, at least in the current > usage configuration. The current environment will only result in two > dictionaries, and nothing besides SDCH is currently using the pref store, so the > asynchronous pref read-in will be under 1K of text (SWAG), and the dictionary > read-in will be O(1M). And it'll probably get to that level pretty quickly :-}. We will probably move other net/ configuration stuff in over time, like information about which servers support SPDY/QUIC. Nothing that should grow much over time, hopefully - the worst case for losing data is worse network performance, so we can be fairly aggressive about tossing out old cruft.
On 2015/02/19 20:47:07, mmenke wrote: > On 2015/02/19 20:39:33, rdsmith wrote: > > On 2015/02/19 20:26:39, gab wrote: > > > From a performance P.O.V. I'm mostly okay with this given the read is > > > asynchronous and off the BlockingPool (which is what I'd like more and more > > > things to be over time), three questions though: > > > > > > 1) Could you possibly delay this further (i.e. although this is > asynchronous, > > IO > > > contention is high on startup and should be avoided at all cost -- we > already > > do > > > way too much of it. Could you delay even kicking off this async read until > > > further down the startup line -- we unfortunately don't have much better > than > > > PostDelayedTask with a random number for this at the moment though... is > > kicking > > > off the read lazily an option for you? If so, how early would the lazy task > > kick > > > off in practice?). > > > > I think it's fine to kick the can a bit further down the startup timeline; > want > > to suggest a random number? I can imagine usage patterns on mobile resulting > in > > this being less than ideal, but I have no interest in making that argument > > without data. > > > > (Side note: I'd love to have a more general way to address this problem; > > dispatch when load under some value? But that's not for this CL.) > > > > > 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON > which > > > results in much bigger payloads than would be required to store the same > data > > in > > > a binary format typically (sure this is a Chrome-wide issue with existing > > > stores, but it would be nice to not add more JSON stores as we evaluate > > whether > > > to replace existing ones). > > > > I picked JSON pretty much for simplicity of programming. I have no long-term > > objection to switching to a different format (I'd like it to be general enough > > to support any reasonable network stack persistence needs, but that still > leaves > > a lot of room). But I'd really like to get this into M42, and I can't imagine > > we could turn around a new persistent store in a day. Is that switch > something > > you'd be ok with doing in a separate CL? > > > > > 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf tests > > > would somewhat trigger this code, but not in a significant way as it won't > > have > > > a large amount of dictionaries stored on disk. Please make sure that the > dirty > > > profile used by startup.warm/cold.dirty tests is forged with a large > database > > > for this storage type (contact erikchen@ for information on how to do this). > > > > So in practical terms in the field the limit on dictionary size is going to be > 2 > > dictionaries of 500k each. That's a hard limit on mobile, and a current > likely > > limit on desktop because Google's the only server using SDCH at the moment. I > > don't know if that changes your perspective on the perf waterfall at all, but > > it's at least useful background information. > > > > > Lastly, if you have trouble answering the above questions, please land this > as > > > an A/B Finch experiment (e.g., controlling whether or not to create/hook the > > > |network_json_store_| in ProfileImplIOData::InitializeInternal) which will > > allow > > > you to see confidence intervals on key startup metrics to determine whether > > this > > > affects startup. (oh and lastly, I assume that these databases will grow > > > overtime and thus that the problem, if there is one, may be invisible at > first > > > and grow over time so we should run such an experiment for a while before > > > deriving any conclusion). > > > > I actually don't think the DBs will grow over time, at least in the current > > usage configuration. The current environment will only result in two > > dictionaries, and nothing besides SDCH is currently using the pref store, so > the > > asynchronous pref read-in will be under 1K of text (SWAG), and the dictionary > > read-in will be O(1M). And it'll probably get to that level pretty quickly > :-}. > > We will probably move other net/ configuration stuff in over time, like > information about which servers support SPDY/QUIC. Nothing that should grow > much over time, hopefully - the worst case for losing data is worse network > performance, so we can be fairly aggressive about tossing out old cruft. Ack (and agreed that even that shouldn't be a big deal), but I took Gabriel's concern as being that a single instance of chrome would, over time, grow the size of this store, as happens for, say, the downloads database.
On 2015/02/19 20:56:45, rdsmith wrote: > On 2015/02/19 20:47:07, mmenke wrote: > > On 2015/02/19 20:39:33, rdsmith wrote: > > > On 2015/02/19 20:26:39, gab wrote: > > > > From a performance P.O.V. I'm mostly okay with this given the read is > > > > asynchronous and off the BlockingPool (which is what I'd like more and > more > > > > things to be over time), three questions though: > > > > > > > > 1) Could you possibly delay this further (i.e. although this is > > asynchronous, > > > IO > > > > contention is high on startup and should be avoided at all cost -- we > > already > > > do > > > > way too much of it. Could you delay even kicking off this async read until > > > > further down the startup line -- we unfortunately don't have much better > > than > > > > PostDelayedTask with a random number for this at the moment though... is > > > kicking > > > > off the read lazily an option for you? If so, how early would the lazy > task > > > kick > > > > off in practice?). > > > > > > I think it's fine to kick the can a bit further down the startup timeline; > > want > > > to suggest a random number? I can imagine usage patterns on mobile > resulting > > in > > > this being less than ideal, but I have no interest in making that argument > > > without data. > > > > > > (Side note: I'd love to have a more general way to address this problem; > > > dispatch when load under some value? But that's not for this CL.) How about 37s?! I wholeheartedly agree that a framework for delayable startup tasks with a way to specify priority, etc. would be awesome and we are thinking about something of the sort at the moment. The one thing we don't want is a notification to tell observers "okay now is the time to do post-startup work" as that would defeat the purpose and re-freeze the browser with tons of "unimportant" tasks -- so from Chrome's birth until now random number delayed tasks were seen like the best way to spread the load (though this is becoming ugly and unmanageable), so I was told by Chrome elders. > > > > > > > 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON > > which > > > > results in much bigger payloads than would be required to store the same > > data > > > in > > > > a binary format typically (sure this is a Chrome-wide issue with existing > > > > stores, but it would be nice to not add more JSON stores as we evaluate > > > whether > > > > to replace existing ones). > > > > > > I picked JSON pretty much for simplicity of programming. I have no > long-term > > > objection to switching to a different format (I'd like it to be general > enough > > > to support any reasonable network stack persistence needs, but that still > > leaves > > > a lot of room). But I'd really like to get this into M42, and I can't > imagine > > > we could turn around a new persistent store in a day. Is that switch > > something > > > you'd be ok with doing in a separate CL? Okay, provided the experiment proves this doesn't hurt startup. Ideally we'd have some kind of generic BinarySerializer to replace JSON uniformly everywhere. PS: I'm not a fan of "landing as-is because we're short on time and need to hit OKRs", at least if it's behind an experiment we can postpone the decision post branch cut when we have data. > > > > > > > 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf > tests > > > > would somewhat trigger this code, but not in a significant way as it won't > > > have > > > > a large amount of dictionaries stored on disk. Please make sure that the > > dirty > > > > profile used by startup.warm/cold.dirty tests is forged with a large > > database > > > > for this storage type (contact erikchen@ for information on how to do > this). > > > > > > So in practical terms in the field the limit on dictionary size is going to > be > > 2 > > > dictionaries of 500k each. That's a hard limit on mobile, and a current > > likely > > > limit on desktop because Google's the only server using SDCH at the moment. > I > > > don't know if that changes your perspective on the perf waterfall at all, > but > > > it's at least useful background information. > > > > > > > Lastly, if you have trouble answering the above questions, please land > this > > as > > > > an A/B Finch experiment (e.g., controlling whether or not to create/hook > the > > > > |network_json_store_| in ProfileImplIOData::InitializeInternal) which will > > > allow > > > > you to see confidence intervals on key startup metrics to determine > whether > > > this > > > > affects startup. (oh and lastly, I assume that these databases will grow > > > > overtime and thus that the problem, if there is one, may be invisible at > > first > > > > and grow over time so we should run such an experiment for a while before > > > > deriving any conclusion). > > > > > > I actually don't think the DBs will grow over time, at least in the current > > > usage configuration. The current environment will only result in two > > > dictionaries, and nothing besides SDCH is currently using the pref store, so > > the > > > asynchronous pref read-in will be under 1K of text (SWAG), and the > dictionary > > > read-in will be O(1M). And it'll probably get to that level pretty quickly > > :-}. > > > > We will probably move other net/ configuration stuff in over time, like > > information about which servers support SPDY/QUIC. Nothing that should grow > > much over time, hopefully - the worst case for losing data is worse network > > performance, so we can be fairly aggressive about tossing out old cruft. > > Ack (and agreed that even that shouldn't be a big deal), but I took Gabriel's > concern as being that a single instance of chrome would, over time, grow the > size of this store, as happens for, say, the downloads database. Ah okay, I misunderstood and thought this was some kind of cache that could grow over time, given that it's not it feels very easy to run an A/B experiment and know the truth in a day? Also makes it sound like modifying the dirty profile used for telemetry will be fairly simple since this data doesn't come in tons of different forms, correct? Please do so, we're trying to minimize the number of things invisible to telemetry.
On 2015/02/19 21:11:43, gab wrote: > On 2015/02/19 20:56:45, rdsmith wrote: > > On 2015/02/19 20:47:07, mmenke wrote: > > > On 2015/02/19 20:39:33, rdsmith wrote: > > > > On 2015/02/19 20:26:39, gab wrote: > > > > > From a performance P.O.V. I'm mostly okay with this given the read is > > > > > asynchronous and off the BlockingPool (which is what I'd like more and > > more > > > > > things to be over time), three questions though: > > > > > > > > > > 1) Could you possibly delay this further (i.e. although this is > > > asynchronous, > > > > IO > > > > > contention is high on startup and should be avoided at all cost -- we > > > already > > > > do > > > > > way too much of it. Could you delay even kicking off this async read > until > > > > > further down the startup line -- we unfortunately don't have much better > > > than > > > > > PostDelayedTask with a random number for this at the moment though... is > > > > kicking > > > > > off the read lazily an option for you? If so, how early would the lazy > > task > > > > kick > > > > > off in practice?). > > > > > > > > I think it's fine to kick the can a bit further down the startup timeline; > > > want > > > > to suggest a random number? I can imagine usage patterns on mobile > > resulting > > > in > > > > this being less than ideal, but I have no interest in making that argument > > > > without data. > > > > > > > > (Side note: I'd love to have a more general way to address this problem; > > > > dispatch when load under some value? But that's not for this CL.) > > > How about 37s?! > > I wholeheartedly agree that a framework for delayable startup tasks with a way > to specify priority, etc. would be awesome and we are thinking about something > of the sort at the moment. The one thing we don't want is a notification to tell > observers "okay now is the time to do post-startup work" as that would defeat > the purpose and re-freeze the browser with tons of "unimportant" tasks -- so > from Chrome's birth until now random number delayed tasks were seen like the > best way to spread the load (though this is becoming ugly and unmanageable), so > I was told by Chrome elders. > > > > > > > > > > > > 2) Does this need to be JSON? The JsonPrefStore uses pretty-printed JSON > > > which > > > > > results in much bigger payloads than would be required to store the same > > > data > > > > in > > > > > a binary format typically (sure this is a Chrome-wide issue with > existing > > > > > stores, but it would be nice to not add more JSON stores as we evaluate > > > > whether > > > > > to replace existing ones). > > > > > > > > I picked JSON pretty much for simplicity of programming. I have no > > long-term > > > > objection to switching to a different format (I'd like it to be general > > enough > > > > to support any reasonable network stack persistence needs, but that still > > > leaves > > > > a lot of room). But I'd really like to get this into M42, and I can't > > imagine > > > > we could turn around a new persistent store in a day. Is that switch > > > something > > > > you'd be ok with doing in a separate CL? > > > Okay, provided the experiment proves this doesn't hurt startup. > > Ideally we'd have some kind of generic BinarySerializer to replace JSON > uniformly everywhere. > > PS: I'm not a fan of "landing as-is because we're short on time and need to hit > OKRs", at least if it's behind an experiment we can postpone the decision post > branch cut when we have data. +1 to this. I also don't really see any reason to rush this out for M42. It's a 1,200 line CL, which I consider makes it the kind of CL you generally don't want to rush in just prior to branch. I'm not going to block it being landed for M42, but I don't think it's a great idea. We get plenty of stats from the other channels, and I don't see this as a performance improvement that can't wait. > > > > > > > > > > > 3) Is this covered by the perf waterfall? i.e. cold/warm startup perf > > tests > > > > > would somewhat trigger this code, but not in a significant way as it > won't > > > > have > > > > > a large amount of dictionaries stored on disk. Please make sure that the > > > dirty > > > > > profile used by startup.warm/cold.dirty tests is forged with a large > > > database > > > > > for this storage type (contact erikchen@ for information on how to do > > this). > > > > > > > > So in practical terms in the field the limit on dictionary size is going > to > > be > > > 2 > > > > dictionaries of 500k each. That's a hard limit on mobile, and a current > > > likely > > > > limit on desktop because Google's the only server using SDCH at the > moment. > > I > > > > don't know if that changes your perspective on the perf waterfall at all, > > but > > > > it's at least useful background information. > > > > > > > > > Lastly, if you have trouble answering the above questions, please land > > this > > > as > > > > > an A/B Finch experiment (e.g., controlling whether or not to create/hook > > the > > > > > |network_json_store_| in ProfileImplIOData::InitializeInternal) which > will > > > > allow > > > > > you to see confidence intervals on key startup metrics to determine > > whether > > > > this > > > > > affects startup. (oh and lastly, I assume that these databases will grow > > > > > overtime and thus that the problem, if there is one, may be invisible at > > > first > > > > > and grow over time so we should run such an experiment for a while > before > > > > > deriving any conclusion). > > > > > > > > I actually don't think the DBs will grow over time, at least in the > current > > > > usage configuration. The current environment will only result in two > > > > dictionaries, and nothing besides SDCH is currently using the pref store, > so > > > the > > > > asynchronous pref read-in will be under 1K of text (SWAG), and the > > dictionary > > > > read-in will be O(1M). And it'll probably get to that level pretty > quickly > > > :-}. > > > > > > We will probably move other net/ configuration stuff in over time, like > > > information about which servers support SPDY/QUIC. Nothing that should grow > > > much over time, hopefully - the worst case for losing data is worse network > > > performance, so we can be fairly aggressive about tossing out old cruft. > > > > Ack (and agreed that even that shouldn't be a big deal), but I took Gabriel's > > concern as being that a single instance of chrome would, over time, grow the > > size of this store, as happens for, say, the downloads database. > > Ah okay, I misunderstood and thought this was some kind of cache that could grow > over time, given that it's not it feels very easy to run an A/B experiment and > know the truth in a day? > > Also makes it sound like modifying the dirty profile used for telemetry will be > fairly simple since this data doesn't come in tons of different forms, correct? > Please do so, we're trying to minimize the number of things invisible to > telemetry.
Another +1 to "it's done when it's done". Marathon not sprint etc On Thu, Feb 19, 2015 at 1:17 PM, <mmenke@chromium.org> wrote: > On 2015/02/19 21:11:43, gab wrote: > >> On 2015/02/19 20:56:45, rdsmith wrote: >> > On 2015/02/19 20:47:07, mmenke wrote: >> > > On 2015/02/19 20:39:33, rdsmith wrote: >> > > > On 2015/02/19 20:26:39, gab wrote: >> > > > > From a performance P.O.V. I'm mostly okay with this given the >> read is >> > > > > asynchronous and off the BlockingPool (which is what I'd like >> more and >> > more >> > > > > things to be over time), three questions though: >> > > > > >> > > > > 1) Could you possibly delay this further (i.e. although this is >> > > asynchronous, >> > > > IO >> > > > > contention is high on startup and should be avoided at all cost >> -- we >> > > already >> > > > do >> > > > > way too much of it. Could you delay even kicking off this async >> read >> until >> > > > > further down the startup line -- we unfortunately don't have much >> > better > >> > > than >> > > > > PostDelayedTask with a random number for this at the moment >> though... >> > is > >> > > > kicking >> > > > > off the read lazily an option for you? If so, how early would the >> lazy >> > task >> > > > kick >> > > > > off in practice?). >> > > > >> > > > I think it's fine to kick the can a bit further down the startup >> > timeline; > >> > > want >> > > > to suggest a random number? I can imagine usage patterns on mobile >> > resulting >> > > in >> > > > this being less than ideal, but I have no interest in making that >> > argument > >> > > > without data. >> > > > >> > > > (Side note: I'd love to have a more general way to address this >> problem; >> > > > dispatch when load under some value? But that's not for this CL.) >> > > > How about 37s?! >> > > I wholeheartedly agree that a framework for delayable startup tasks with >> a way >> to specify priority, etc. would be awesome and we are thinking about >> something >> of the sort at the moment. The one thing we don't want is a notification >> to >> > tell > >> observers "okay now is the time to do post-startup work" as that would >> defeat >> the purpose and re-freeze the browser with tons of "unimportant" tasks -- >> so >> from Chrome's birth until now random number delayed tasks were seen like >> the >> best way to spread the load (though this is becoming ugly and >> unmanageable), >> > so > >> I was told by Chrome elders. >> > > > > > > > >> > > > > 2) Does this need to be JSON? The JsonPrefStore uses >> pretty-printed >> > JSON > >> > > which >> > > > > results in much bigger payloads than would be required to store >> the >> > same > >> > > data >> > > > in >> > > > > a binary format typically (sure this is a Chrome-wide issue with >> existing >> > > > > stores, but it would be nice to not add more JSON stores as we >> > evaluate > >> > > > whether >> > > > > to replace existing ones). >> > > > >> > > > I picked JSON pretty much for simplicity of programming. I have no >> > long-term >> > > > objection to switching to a different format (I'd like it to be >> general >> > enough >> > > > to support any reasonable network stack persistence needs, but that >> > still > >> > > leaves >> > > > a lot of room). But I'd really like to get this into M42, and I >> can't >> > imagine >> > > > we could turn around a new persistent store in a day. Is that >> switch >> > > something >> > > > you'd be ok with doing in a separate CL? >> > > > Okay, provided the experiment proves this doesn't hurt startup. >> > > Ideally we'd have some kind of generic BinarySerializer to replace JSON >> uniformly everywhere. >> > > PS: I'm not a fan of "landing as-is because we're short on time and need >> to >> > hit > >> OKRs", at least if it's behind an experiment we can postpone the decision >> post >> branch cut when we have data. >> > > +1 to this. I also don't really see any reason to rush this out for M42. > It's > a 1,200 line CL, which I consider makes it the kind of CL you generally > don't > want to rush in just prior to branch. I'm not going to block it being > landed > for M42, but I don't think it's a great idea. We get plenty of stats from > the > other channels, and I don't see this as a performance improvement that > can't > wait. > > > > > > > > >> > > > > 3) Is this covered by the perf waterfall? i.e. cold/warm startup >> perf >> > tests >> > > > > would somewhat trigger this code, but not in a significant way as >> it >> won't >> > > > have >> > > > > a large amount of dictionaries stored on disk. Please make sure >> that >> > the > >> > > dirty >> > > > > profile used by startup.warm/cold.dirty tests is forged with a >> large >> > > database >> > > > > for this storage type (contact erikchen@ for information on how >> to do >> > this). >> > > > >> > > > So in practical terms in the field the limit on dictionary size is >> going >> to >> > be >> > > 2 >> > > > dictionaries of 500k each. That's a hard limit on mobile, and a >> current >> > > likely >> > > > limit on desktop because Google's the only server using SDCH at the >> moment. >> > I >> > > > don't know if that changes your perspective on the perf waterfall at >> > all, > >> > but >> > > > it's at least useful background information. >> > > > >> > > > > Lastly, if you have trouble answering the above questions, please >> land >> > this >> > > as >> > > > > an A/B Finch experiment (e.g., controlling whether or not to >> > create/hook > >> > the >> > > > > |network_json_store_| in ProfileImplIOData::InitializeInternal) >> which >> will >> > > > allow >> > > > > you to see confidence intervals on key startup metrics to >> determine >> > whether >> > > > this >> > > > > affects startup. (oh and lastly, I assume that these databases >> will >> > grow > >> > > > > overtime and thus that the problem, if there is one, may be >> invisible >> > at > >> > > first >> > > > > and grow over time so we should run such an experiment for a while >> before >> > > > > deriving any conclusion). >> > > > >> > > > I actually don't think the DBs will grow over time, at least in the >> current >> > > > usage configuration. The current environment will only result in >> two >> > > > dictionaries, and nothing besides SDCH is currently using the pref >> > store, > >> so >> > > the >> > > > asynchronous pref read-in will be under 1K of text (SWAG), and the >> > dictionary >> > > > read-in will be O(1M). And it'll probably get to that level pretty >> quickly >> > > :-}. >> > > >> > > We will probably move other net/ configuration stuff in over time, >> like >> > > information about which servers support SPDY/QUIC. Nothing that >> should >> > grow > >> > > much over time, hopefully - the worst case for losing data is worse >> > network > >> > > performance, so we can be fairly aggressive about tossing out old >> cruft. >> > >> > Ack (and agreed that even that shouldn't be a big deal), but I took >> > Gabriel's > >> > concern as being that a single instance of chrome would, over time, >> grow the >> > size of this store, as happens for, say, the downloads database. >> > > Ah okay, I misunderstood and thought this was some kind of cache that >> could >> > grow > >> over time, given that it's not it feels very easy to run an A/B >> experiment and >> know the truth in a day? >> > > Also makes it sound like modifying the dirty profile used for telemetry >> will >> > be > >> fairly simple since this data doesn't come in tons of different forms, >> > correct? > >> Please do so, we're trying to minimize the number of things invisible to >> telemetry. >> > > > > https://codereview.chromium.org/901303002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); On 2015/02/19 20:14:35, mmenke wrote: > Also, if we're still loading something from the cache on startup, and then we > see a real request try to use it, calling Schedule, the metadata from the new > fetch will completely overwrite the same info loaded from the pref file, if the > cache load succeeds (use count, etc). That seems kinda weird. And just to clarify, this wouldn't happen if fetch_queue_ contained the current fetch. Also, if the order were reversed - we do the "reload" after the non-reload request, the reload will always be cancelled in favor of the load, so we lose the meta-data from the pref file in that case, too. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:240: DCHECK_NE(0, outstanding_jobs_delta); Include base/logging.h for DCHECK https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:338: TEST_F(SdchDictionaryFetcherTest, ScheduleReloadLoadFlags) { Should probably have a test where we have a couple tests where a reload and a non-reload are scheduled at once - reload first, and non-reload done while it's in progress, reload second, and done while the non-reload is in progress, and maybe also a pair where the second of the two requests comes in while the other request is still in the queue. This is pretty much along the same lines as my comment about testing how we handle attempted_load_. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:344: std::vector<DictionaryAdditions> additions; Should include <vector>
New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org,thakis@chromium.org
https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:167: // is iterating over does not export begin()/end() methods. On 2015/02/17 20:57:41, rdsmith wrote: > Could you also add a note to this comment that this iterator should only be used > on a sanitized pref-store, i.e. not on the just-loaded-from-disk prefstore, as > it assumes a specific format for the pref store? Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:84: int GetDictionaryCount() const; On 2015/02/17 20:57:42, rdsmith wrote: > Hmmm. This is only used for testing, and the information should be more > naturally available from the Manager. Willing to add a ForTesting to the end? Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:84: int GetDictionaryCount() const; On 2015/02/19 20:14:35, mmenke wrote: > On 2015/02/17 20:57:42, rdsmith wrote: > > Hmmm. This is only used for testing, and the information should be more > > naturally available from the Manager. Willing to add a ForTesting to the end? > > +1 Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:87: bool HasDictionaryFrom(const GURL& url) const; On 2015/02/17 20:57:42, rdsmith wrote: > Same here (only used for testing, information is more naturally IMO gotten from > the manager, add a "ForTesting"?) Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:87: bool HasDictionaryFrom(const GURL& url) const; On 2015/02/19 20:14:35, mmenke wrote: > On 2015/02/17 20:57:42, rdsmith wrote: > > Same here (only used for testing, information is more naturally IMO gotten > from > > the manager, add a "ForTesting"?) > > +1 Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... File net/sdch/sdch_owner_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:24: bool GetDictionaryForURL(TestingPrefStore* store, const GURL& url, On 2015/02/19 16:25:32, Alexei Svitkine wrote: > Nit: 1 param per line when params wrap. Done. https://codereview.chromium.org/901303002/diff/140001/net/sdch/sdch_owner_uni... net/sdch/sdch_owner_unittest.cc:217: }; On 2015/02/19 16:25:32, Alexei Svitkine wrote: > Nit: DISALLOW_ Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:12: On 2015/02/19 20:14:35, mmenke wrote: > Remove blank line Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:174: attempted_load_.insert(dictionary_url); On 2015/02/19 20:14:35, mmenke wrote: > This should have an explanatory comment. Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:194: next_state_ = STATE_SEND_REQUEST; On 2015/02/19 20:14:35, mmenke wrote: > I thought we were going to remove the next_state_ thing from here? Assuming > clearing the current request means we're ready to start the next one makes me > very nervous. I don't think so? We removed the DoLoop invocation from here, but resetting a request *should* mean we're ready to start the next one, I think. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:77: }; On 2015/02/19 16:25:32, Alexei Svitkine wrote: > Nit: DISALLOW()? Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:77: }; On 2015/02/19 20:14:35, mmenke wrote: > On 2015/02/19 16:25:32, Alexei Svitkine wrote: > > Nit: DISALLOW()? > > And include the header for it. Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:240: DCHECK_NE(0, outstanding_jobs_delta); On 2015/02/19 21:44:58, mmenke wrote: > Include base/logging.h for DCHECK Done. https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:344: std::vector<DictionaryAdditions> additions; On 2015/02/19 21:44:58, mmenke wrote: > Should include <vector> Done.
https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/140001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:194: next_state_ = STATE_SEND_REQUEST; On 2015/02/20 20:35:25, Elly Jones wrote: > On 2015/02/19 20:14:35, mmenke wrote: > > I thought we were going to remove the next_state_ thing from here? Assuming > > clearing the current request means we're ready to start the next one makes me > > very nervous. > > I don't think so? We removed the DoLoop invocation from here, but resetting a > request *should* mean we're ready to start the next one, I think. I think this is a completely unexpected side effect in a method named "ResetRequest" and makes the transition logic for each state much harder to follow.
Quick comments. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:397: EXPECT_TRUE(fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback())); This shouldn't happen. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:403: EXPECT_TRUE(fetcher()->Schedule(dictionary_url, GetDefaultCallback())); Should make sure we actually do a cached load, and if that fails, one that bypasses the cache. BUG: We do in fact do that, but if we do a ScheduleReload with a different dictionary first, we only do a load from the cache, I believe. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:409: EXPECT_FALSE(fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback())); Should check that we actually only issue one request over the network, too, too, just in case. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:416: fetcher()->Cancel(); Should make sure we cancel the pending network request. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:417: EXPECT_TRUE(fetcher()->Schedule(dictionary_url, GetDefaultCallback())); Again, should make sure this really issues a network request.
LGTM. All "suggestions" below are up to you, as it the finch expansion, and looking at them I'll leave the DCHECK and explanatory comment requests up to you as well :-}. https://codereview.chromium.org/901303002/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/901303002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:453: network_json_store_->ReadPrefsAsync(nullptr); Given that there's concern around the costs of this during startup, as well as actually loading in the dictionaries, would it be worthwhile expanding the Finch trial to have a group that doesn't do the async prefs read as well? https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:97: UMA_HISTOGRAM_COUNTS_100("Sdch3.DictionaryUseCount", 0); nit, suggestion: I think of it as a not-great idea to have the same string constant in two different places, just because it might get out of sync. Separately, I wince a bit at the same histogram being called at two different places, since histogram calls initializes static pointers from that location pointing at the histogram (though I think it's correct). This makes me want to re-visit the question of combining these two routines in a function with some name that indicates the range of possible calls for it. But still up to you--I just thought about the string constant/histogram pointer issue on this read through and wanted to share. https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:147: std::map<std::string, int> use_counts_at_load_; Explanatory comment? (I imagine someone who hasn't had the discussions around UMA going "Why would that be a useful member variable?" which suggests to me a comment would be useful.) https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:74: void ForgetPriorFetches(); nit, suggestion: Name change to Clear()? https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; nit, suggestion: This is a bit wasteful of space; how about a map:GURL->(bool,bool) or enum? https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:96: } suggestion: I'd find this code easier to read if it cascaded failure (false) returns, i.e.: if (ever_fetched_.count(info.url) != 0) return false; if (info.cache_only && ever_fetched_.cache(info.url)) return false; // Insert into appropriate queue. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:289: fetch_queue_->Pop(&info); DCHECK that return value is true? https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:106: scoped_ptr<UniqueFetchQueue> fetch_queue_; nit, suggestion: If you declare UniqueFetchQueue inline as a nested class, you don't need to make this a scoped_ptr<> but can instead just have a UniqueFetchQueue as a member variable. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:257: // Last load flags seen by the interceptor installed in nit, suggestion: It reads cleaner to me if there's a blank line before comments line this, and after the member variables the comment refers to.
https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:51: GURL url; blank line before url. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:75: private: nit: Blank line before private. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:76: std::list<FetchInfo> queue_; Any reason this is a list and not a queue? Also, you're including <queue> and not <list> in the header. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:77: std::set<GURL> ever_cache_fetched_; Do we even need this? We load a dictionary of URLs, don't we? So we never end up double-cache-fetching a URL, right? https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: This is a bit wasteful of space; how about a > map:GURL->(bool,bool) or enum? +1. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; The fact that something that was just fetched from the cache was not "ever_fetched" is not at all intuitive. Should document that. Should also document that "fetched" means "fetch queued", rather than "fetch attempted" or "fetched successfully". https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:92: if (ever_fetched_.count(info.url) != 0) { optional: Think it's a bit clearer if we do this first, before the cache only check. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:94: } nit: Don't use braces with single-line if's. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:151: current_callback_.Reset(); This looks a lot like ResetRequest...In fact, we're doing the exact same thing...Except next_state_ is different. We should use it and get rid of the next_state_ transition in ResetRequest. Or rename ResetRequest to ResetRequestAndPrepareForNextRequest or something. Sorry to harp so much on this, but having something name "ResetRequest" do something else, which is something you want to do 3 out of 4 times when reset the request just seems like a bad idea. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:182: } nit: Don't use braces on 2 line ifs. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:15: #include <set> These both can be moved into the CC file, unless you move your new class into the header. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:35: class UniqueFetchQueue; Shouldn't put anonymous namespaces in header files. Instead, should make this a private class of the fetcher. Could either define the class in the header, per Randy's suggestion, or just forward declare it. I'm fine with either. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:80: DISALLOW_COPY_AND_ASSIGN(URLRequestSpecifiedResponseJob); nit: Blank line before DISALLOW... https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:271: DISALLOW_COPY_AND_ASSIGN(SdchDictionaryFetcherTest); Blank line before DISALLOW_COPY_AND_ASSIGN. Also suggest a blank line before the weak pointer factory, to make it stick out a bit more, since it should always go last. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:429: fetcher()->Cancel(); Maybe WaitForNoJobs(), or check that there are no jobs here?
mmenke: ptal? https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:397: EXPECT_TRUE(fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback())); On 2015/02/20 21:58:12, mmenke wrote: > This shouldn't happen. Done. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:403: EXPECT_TRUE(fetcher()->Schedule(dictionary_url, GetDefaultCallback())); On 2015/02/20 21:58:12, mmenke wrote: > Should make sure we actually do a cached load, and if that fails, one that > bypasses the cache. > > BUG: We do in fact do that, but if we do a ScheduleReload with a different > dictionary first, we only do a load from the cache, I believe. Done. https://codereview.chromium.org/901303002/diff/200001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:409: EXPECT_FALSE(fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback())); On 2015/02/20 21:58:12, mmenke wrote: > Should check that we actually only issue one request over the network, too, too, > just in case. Done. https://codereview.chromium.org/901303002/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/901303002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:453: network_json_store_->ReadPrefsAsync(nullptr); On 2015/02/23 21:23:49, rdsmith wrote: > Given that there's concern around the costs of this during startup, as well as > actually loading in the dictionaries, would it be worthwhile expanding the Finch > trial to have a group that doesn't do the async prefs read as well? Talked about this offline and decided not to do it. https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:97: UMA_HISTOGRAM_COUNTS_100("Sdch3.DictionaryUseCount", 0); On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: I think of it as a not-great idea to have the same string > constant in two different places, just because it might get out of sync. > Separately, I wince a bit at the same histogram being called at two different > places, since histogram calls initializes static pointers from that location > pointing at the histogram (though I think it's correct). This makes me want to > re-visit the question of combining these two routines in a function with some > name that indicates the range of possible calls for it. But still up to you--I > just thought about the string constant/histogram pointer issue on this read > through and wanted to share. Done. https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/901303002/diff/240001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:147: std::map<std::string, int> use_counts_at_load_; On 2015/02/23 21:23:49, rdsmith wrote: > Explanatory comment? (I imagine someone who hasn't had the discussions around > UMA going "Why would that be a useful member variable?" which suggests to me a > comment would be useful.) Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:51: GURL url; On 2015/02/26 17:27:20, mmenke wrote: > blank line before url. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:74: void ForgetPriorFetches(); On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: Name change to Clear()? Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:75: private: On 2015/02/26 17:27:20, mmenke wrote: > nit: Blank line before private. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:76: std::list<FetchInfo> queue_; On 2015/02/26 17:27:20, mmenke wrote: > Any reason this is a list and not a queue? Also, you're including <queue> and > not <list> in the header. Oops, it was meant to be a queue. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:77: std::set<GURL> ever_cache_fetched_; On 2015/02/26 17:27:20, mmenke wrote: > Do we even need this? We load a dictionary of URLs, don't we? So we never end > up double-cache-fetching a URL, right? Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: This is a bit wasteful of space; how about a > map:GURL->(bool,bool) or enum? Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; On 2015/02/26 17:27:20, mmenke wrote: > The fact that something that was just fetched from the cache was not > "ever_fetched" is not at all intuitive. Should document that. Should also > document that "fetched" means "fetch queued", rather than "fetch attempted" or > "fetched successfully". Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; On 2015/02/26 17:27:20, mmenke wrote: > > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:78: std::set<GURL> ever_fetched_; On 2015/02/26 17:27:20, mmenke wrote: > On 2015/02/23 21:23:49, rdsmith wrote: > > nit, suggestion: This is a bit wasteful of space; how about a > > map:GURL->(bool,bool) or enum? > > +1. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:92: if (ever_fetched_.count(info.url) != 0) { On 2015/02/26 17:27:20, mmenke wrote: > optional: Think it's a bit clearer if we do this first, before the cache only > check. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:94: } On 2015/02/26 17:27:20, mmenke wrote: > nit: Don't use braces with single-line if's. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:96: } On 2015/02/23 21:23:49, rdsmith wrote: > suggestion: I'd find this code easier to read if it cascaded failure (false) > returns, i.e.: > > if (ever_fetched_.count(info.url) != 0) > return false; > > if (info.cache_only && ever_fetched_.cache(info.url)) > return false; > > // Insert into appropriate queue. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:151: current_callback_.Reset(); On 2015/02/26 17:27:20, mmenke wrote: > This looks a lot like ResetRequest...In fact, we're doing the exact same > thing...Except next_state_ is different. We should use it and get rid of the > next_state_ transition in ResetRequest. Or rename ResetRequest to > ResetRequestAndPrepareForNextRequest or something. > > Sorry to harp so much on this, but having something name "ResetRequest" do > something else, which is something you want to do 3 out of 4 times when reset > the request just seems like a bad idea. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:182: } On 2015/02/26 17:27:20, mmenke wrote: > nit: Don't use braces on 2 line ifs. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:289: fetch_queue_->Pop(&info); On 2015/02/23 21:23:49, rdsmith wrote: > DCHECK that return value is true? Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:15: #include <set> On 2015/02/26 17:27:20, mmenke wrote: > These both can be moved into the CC file, unless you move your new class into > the header. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:35: class UniqueFetchQueue; On 2015/02/26 17:27:21, mmenke wrote: > Shouldn't put anonymous namespaces in header files. Instead, should make this a > private class of the fetcher. Could either define the class in the header, per > Randy's suggestion, or just forward declare it. I'm fine with either. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.h:106: scoped_ptr<UniqueFetchQueue> fetch_queue_; On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: If you declare UniqueFetchQueue inline as a nested class, you > don't need to make this a scoped_ptr<> but can instead just have a > UniqueFetchQueue as a member variable. I used the scoped_ptr to avoid inlining it. I've moved the forward declaration of UniqueFetchQueue into this class, though. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:80: DISALLOW_COPY_AND_ASSIGN(URLRequestSpecifiedResponseJob); On 2015/02/26 17:27:21, mmenke wrote: > nit: Blank line before DISALLOW... Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:257: // Last load flags seen by the interceptor installed in On 2015/02/23 21:23:49, rdsmith wrote: > nit, suggestion: It reads cleaner to me if there's a blank line before comments > line this, and after the member variables the comment refers to. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:271: DISALLOW_COPY_AND_ASSIGN(SdchDictionaryFetcherTest); On 2015/02/26 17:27:21, mmenke wrote: > Blank line before DISALLOW_COPY_AND_ASSIGN. Also suggest a blank line before > the weak pointer factory, to make it stick out a bit more, since it should > always go last. Done. https://codereview.chromium.org/901303002/diff/240001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:429: fetcher()->Cancel(); On 2015/02/26 17:27:21, mmenke wrote: > Maybe WaitForNoJobs(), or check that there are no jobs here? Done.
mmenke: ptal?
LGTM! https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:79: DISALLOW_COPY_AND_ASSIGN(UniqueFetchQueue); nit: Blank line before DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:400: TEST_F(SdchDictionaryFetcherTest, ScheduleTwoReloads) { nit: Don't think we need this test
https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:79: DISALLOW_COPY_AND_ASSIGN(UniqueFetchQueue); On 2015/03/03 21:51:53, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/901303002/diff/260001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:400: TEST_F(SdchDictionaryFetcherTest, ScheduleTwoReloads) { On 2015/03/03 21:51:53, mmenke wrote: > nit: Don't think we need this test Done.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, thakis@chromium.org, asvitkine@chromium.org, rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/901303002/#ps280001 (title: "Last round")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901303002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
lgtm (w/ a few drive-by nits below), thanks for adding it as an experiment. One question about the experiment: I don't see any new histograms being added in this CL, do you have histograms on things you hope to improve by adding this extra caching? i.e., to justify enabling this IMO not only do you need to prove it doesn't hinder startup, but should also prove that the caching results in noticeable wins in whichever metric it's trying to optimize (in an effort to reduce IO we want to make sure any IO we do is justified and that the trade-offs, if any, are well understood). Thanks, Gab https://codereview.chromium.org/901303002/diff/280001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/901303002/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:99: base::FieldTrialList::FindFullName("SdchPersistence"); nit: This is not needed until after you've handled the command-line, more it at the end of this method. https://codereview.chromium.org/901303002/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:107: } nit: Can remove {} for one-line ifs. https://codereview.chromium.org/901303002/diff/280001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:645: } nit: Remove {}
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, gab@chromium.org, rdsmith@chromium.org, thakis@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/901303002/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901303002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, gab@chromium.org, rdsmith@chromium.org, thakis@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/901303002/#ps320001 (title: "Fix cache control headers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901303002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/d0ff2559cf187088623898f0b2dd95841123cf6c Cr-Commit-Position: refs/heads/master@{#319753} |
