Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1051353003:
SDCH: add TimeWeightedMemoryUse (Closed)
|
Created:
5 years, 8 months ago by Elly Fong-Jones Modified:
5 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSDCH: add TimeWeightedMemoryUse
This histogram tracks the time-weighted memory use incurred by SDCH
dictionaries. This histogram is measured in units of byte-seconds per
second.
BUG=465873
Committed: https://crrev.com/d5809f856236b6c7273470b4b7721269a083b148
Cr-Commit-Position: refs/heads/master@{#327342}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Improve test coverage #
Total comments: 6
Patch Set 3 : Change metric denominator #
Total comments: 2
Patch Set 4 : Record more exact creation time #
Total comments: 2
Patch Set 5 : Add missing .erase() #Patch Set 6 : Histogram tweaks #Patch Set 7 : Stop using CurrentProcessInfo #Patch Set 8 : Don't divide by zero #Patch Set 9 : Don't reset clock #
Messages
Total messages: 39 (18 generated)
ellyjones@chromium.org changed reviewers: + rdsmith@chromium.org
ptal?
A couple of bits of confusion; I may be missing something basic. Did you/are you doing manual tests with these histograms to make sure the results look like you expect them too? (Since the automated tests only test for the presence of entries.) https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:191: base::Time load_time_; I'm probably missing something really obvious, but why this member variable? I don't see it ever being set, and my memory of the design you settled on was that load_time_ wouldn't be stored in the persistent store. (Doing the rest of the review assuming that this is a leftover bit of cruft from an earlier implementation.) https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:404: it.load_time())); Shouldn't this be a lookup in the load_times_ map? Though actually, I don't think the value will ever be used; should the load time be in the structure at all? https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:795: if (load_times_.count(server_hash) == 0) When will this happen? Shouldn't this be a DCHECK? Or the "return;" a "NOTREACHED(); return;"?
I improved the test so it now checks that the histogram is logging a sensible value. https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:191: base::Time load_time_; On 2015/04/03 13:25:29, rdsmith wrote: > I'm probably missing something really obvious, but why this member variable? I > don't see it ever being set, and my memory of the design you settled on was that > load_time_ wouldn't be stored in the persistent store. > > (Doing the rest of the review assuming that this is a leftover bit of cruft from > an earlier implementation.) Yeah, this was vestigal. Removed. https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:404: it.load_time())); On 2015/04/03 13:25:29, rdsmith wrote: > Shouldn't this be a lookup in the load_times_ map? Though actually, I don't > think the value will ever be used; should the load time be in the structure at > all? Done. https://codereview.chromium.org/1051353003/diff/1/net/sdch/sdch_owner.cc#newc... net/sdch/sdch_owner.cc:795: if (load_times_.count(server_hash) == 0) On 2015/04/03 13:25:29, rdsmith wrote: > When will this happen? Shouldn't this be a DCHECK? Or the "return;" a > "NOTREACHED(); return;"? Done.
Top level comment: I feel like describing the units as "bytes-seconds per second" isn't accurate; the units are bytes (== average cost of SDCH dictionaries over browser lifetime), and the way we get those bytes is by dividing the byte-seconds of SDCH usage by the seconds of browser lifetime. https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:356: base::Time load_time; ?? https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:728: DICTIONARY_FATE_EVICT_FOR_MEMORY); Why isn't there a RecordDictionaryLifetime here? More generally, any reason not to combine that call with this one? I think they'd generally both be called at the same time. https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:794: base::CurrentProcessInfo::CreationTime(); I'm not sure this is the right value. Specifically, imagine that we start up chrome, do a whole lot of dictionary loading and use, clear dictionaries, then use chrome for a long while longer. Our "average bytes used over chrome's lifetime" value will be way off. Another way to put it is that this biases things a lot by when the dictionary gets kicked out of memory in relationship to the process shutting down. I'm inclined to think that this means we need to record the statistic at SdchOwner destruction. That's still not ideal (Incognito, other profiles) but I think it'd be mostly right.
https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:356: base::Time load_time; On 2015/04/03 16:08:18, rdsmith wrote: > ?? Done. https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:728: DICTIONARY_FATE_EVICT_FOR_MEMORY); On 2015/04/03 16:08:18, rdsmith wrote: > Why isn't there a RecordDictionaryLifetime here? More generally, any reason not > to combine that call with this one? I think they'd generally both be called at > the same time. Done. https://codereview.chromium.org/1051353003/diff/20001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:794: base::CurrentProcessInfo::CreationTime(); On 2015/04/03 16:08:18, rdsmith wrote: > I'm not sure this is the right value. Specifically, imagine that we start up > chrome, do a whole lot of dictionary loading and use, clear dictionaries, then > use chrome for a long while longer. Our "average bytes used over chrome's > lifetime" value will be way off. Another way to put it is that this biases > things a lot by when the dictionary gets kicked out of memory in relationship to > the process shutting down. > > I'm inclined to think that this means we need to record the statistic at > SdchOwner destruction. That's still not ideal (Incognito, other profiles) but I > think it'd be mostly right. Done.
LGTM modulo the SdchOwner lifetime comment below. https://codereview.chromium.org/1051353003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/40001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:235: DCHECK(load_times_.count(server_hash) == 1); If you're going to have this DCHECK, that suggests to me that you should also delete the entry in load_times_ at the end of this function. Otherwise the DCHECK() isn't checking anything if a dictionary is re-loaded. (Just a suggestion, may be irrelevant because SdchDictionaryFetcher won't re-fetch the same dictionary without a clear, but I'd consider that a bug if it's true and that behavior isn't one SdchOwner should count on anyway. But it's just a DCHECK.) https://codereview.chromium.org/1051353003/diff/40001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:294: base::Time creation_time = base::CurrentProcessInfo::CreationTime(); I'm still inclined to think this should just be the lifetime of the SdchOwner; that seems strictly better than process lifetime. WDYT?
ellyjones@chromium.org changed reviewers: + isherman@chromium.org
LGTM https://codereview.chromium.org/1051353003/diff/60001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/1051353003/diff/60001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:296: for (auto& val : consumed_byte_seconds_) { nit: "const auto&"? https://codereview.chromium.org/1051353003/diff/60001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:298: val / process_lifetime); Optional: Maybe use UMA_HISTOGRAM_MEMORY_KB here?
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1051353003/#ps100001 (title: "Histogram tweaks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1051353003/#ps120001 (title: "Stop using CurrentProcessInfo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1051353003/#ps140001 (title: "Don't divide by zero")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/140001
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1051353003/#ps160001 (title: "Don't reset clock")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051353003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d5809f856236b6c7273470b4b7721269a083b148 Cr-Commit-Position: refs/heads/master@{#327342} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
