Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073523003:
Add tagged histogram with reasons of purging the dom storage databases (Closed)
DescriptionAdd tagged histogram with reasons of purging the dom storage databases
The existing metric shows us useful information on how much cache is
purged. But, we do not know about the reason for purging. Since there a
lot of cases where we are purging less than 10KB, it would be better to
understand this case and try to avoid triggering purge code.
BUG=610551
Committed: https://crrev.com/cc032854d31ede369b1cfb2090ea3abf55538630
Cr-Commit-Position: refs/heads/master@{#400847}
Patch Set 1 #
Total comments: 2
Patch Set 2 : nit. #Patch Set 3 : Re-arrange ifs and add histogram names. #
Messages
Total messages: 23 (7 generated)
ssid@chromium.org changed reviewers: + mariakhomenko@chromium.org
Maria, could you please review this CL? Not sure if this is the right way to add tags. the existing metrics are here: http://shortn/_hWNFA3ft94 Though it is seen that purging is useful 50% (purges more than 100K) it will still be good to reduce the 0 cases.
lgtm https://codereview.chromium.org/2073523003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_impl.cc (right): https://codereview.chromium.org/2073523003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_impl.cc:471: std::string("LocalStorage.BrowserLocalStorageCachePurgedInKB.") + you also need to update histograms.xml file with the new names.
https://codereview.chromium.org/2073523003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_impl.cc (right): https://codereview.chromium.org/2073523003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_impl.cc:471: std::string("LocalStorage.BrowserLocalStorageCachePurgedInKB.") + On 2016/06/16 00:08:15, Maria wrote: > you also need to update histograms.xml file with the new names. I copied the pattern from "Sqlite.SizeKB" it also seems to add multiple histograms with a "." after the name similar to what i have done here. The dashboard adds the existing description with the last part of name. Like https://uma.googleplex.com/p/chrome/histograms/?endDate=06-14-2016&dayCount=1...
ssid@chromium.org changed reviewers: + michaeln@chromium.org
+michaeln ptal, The existing metrics are here: http://shortn/_hWNFA3ft94 Though it is seen that purging is useful 50% (purges more than 100K) it will still be good to reduce the 0 cases to avoid perf overhead of trying to purge.
Will we learn anything actionable from these new metrics? At what frequency is purge is invoked? Is it a problem that often there's nothing to purge? Would an optimization like if (PURGE_IF_NEEDED && total_cache_size < kNotWorthPurging) return alleviate the problem your trying to analyze? I think there is extra stuff to handling the labeling, i see the following for Sqlite metrics? Maybe ask isherman about how to handle the suffixes? <histogram_suffixes name="SqliteDatabases" separator="."> <owner>shess@chromium.org</owner> <suffix name="Activity" label="Activity"/> <suffix name="Affiliation" label="Affiliation"/> <suffix name="AppCache" label="AppCache"/> <suffix name="BookmarkImages" label="BookmarkImages"/> <suffix name="Cookie" label="Cookie"/> <suffix name="DatabaseTracker" label="DatabaseTracker"/> <suffix name="DomainBoundCerts" label="DomainBoundCerts"/> <suffix name="DOMStorageDatabase" label="DOMStorageDatabase"/> <suffix name="History" label="History"/> <suffix name="OfflinePageMetadata" label="OfflinePageMetadata"/> <suffix name="Passwords" label="Passwords"/> <suffix name="Predictor" label="Predictor"/> <suffix name="Quota" label="Quota"/> <suffix name="Shortcuts" label="Shortcuts"/> <suffix name="SyncDirectory" label="SyncDirectory"/> <suffix name="Text" label="Text (obsolete 7/24/13)"/> <suffix name="Thumbnail" label="Thumbnail"/> <suffix name="TopSites" label="TopSites"/> <suffix name="Web" label="Web"/> <affected-histogram name="Sqlite.AutoCommitTime"/> <affected-histogram name="Sqlite.CommitTime"/> <affected-histogram name="Sqlite.Error"/> <affected-histogram name="Sqlite.QueryTime"/> <affected-histogram name="Sqlite.SizeKB"/> <affected-histogram name="Sqlite.Stats"/> <affected-histogram name="Sqlite.UpdateTime"/> <affected-histogram name="Sqlite.Version"/> </histogram_suffixes>
On 2016/06/17 19:28:39, michaeln wrote: > Will we learn anything actionable from these new metrics? We are hoping to figure out whether adding new purge conditions was useful in comparison to the old code. So we'd like to know whether purging on low end devices generally gives us useful results, etc.
On 2016/06/17 19:52:15, Maria wrote: > On 2016/06/17 19:28:39, michaeln wrote: > > Will we learn anything actionable from these new metrics? > > We are hoping to figure out whether adding new purge conditions was useful in > comparison to the old code. So we'd like to know whether purging on low end > devices generally gives us useful results, etc. Would it make sense to have buckets like this then? LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive.LowEndDevice LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded.LowEndDevice That might make it easier see what's happening on "low end" devices specifically.
On 2016/06/17 20:36:08, michaeln wrote: > On 2016/06/17 19:52:15, Maria wrote: > > On 2016/06/17 19:28:39, michaeln wrote: > > > Will we learn anything actionable from these new metrics? > > > > We are hoping to figure out whether adding new purge conditions was useful in > > comparison to the old code. So we'd like to know whether purging on low end > > devices generally gives us useful results, etc. > > Would it make sense to have buckets like this then? > > LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive > LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded > LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive.LowEndDevice > LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded.LowEndDevice > > That might make it easier see what's happening on "low end" devices > specifically. There is no need to record for low end devices separately -- UMA has a filter for low end devices. But we do want to know which condition caused the purge there.
On 2016/06/17 20:40:56, Maria wrote: > On 2016/06/17 20:36:08, michaeln wrote: > > On 2016/06/17 19:52:15, Maria wrote: > > > On 2016/06/17 19:28:39, michaeln wrote: > > > > Will we learn anything actionable from these new metrics? > > > > > > We are hoping to figure out whether adding new purge conditions was useful > in > > > comparison to the old code. So we'd like to know whether purging on low end > > > devices generally gives us useful results, etc. > > > > Would it make sense to have buckets like this then? > > > > LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive > > LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded > > LocalStorage.BrowserLocalStorageCachePurgedInKB.Agressive.LowEndDevice > > LocalStorage.BrowserLocalStorageCachePurgedInKB.IfNeeded.LowEndDevice > > > > That might make it easier see what's happening on "low end" devices > > specifically. > > There is no need to record for low end devices separately -- UMA has a filter > for low end devices. But we do want to know which condition caused the purge > there. Ok, I think the if else conditions need rearranging to see the full set, otherwise the pair of exceeded reasons will never get logged for low end devices. That and the histogram.xml changes and this looks good. LocalStorage.BrowserLocalStorageCachePurgedInKB.InactiveOnLowEndDevice LocalStorage.BrowserLocalStorageCachePurgedInKB.SizeLimitExceeded LocalStorage.BrowserLocalStorageCachePurgedInKB.AreaCountLimitExceeded LocalStorage.BrowserLocalStorageCachePurgedInKB.MemoryPressureSignal Given the finer grained stats, do we still want the general LocalStorage.BrowserLocalStorageCachePurgedInKB that gets logged regardless of the reason?
On 2016/06/17 19:28:39, michaeln wrote: > Will we learn anything actionable from these new metrics? At what frequency is > purge is invoked? Is it a problem that often there's nothing to purge? Would an > optimization like if (PURGE_IF_NEEDED && total_cache_size < kNotWorthPurging) > return alleviate the problem your trying to analyze? Yes, that's a good idea, I wanted to implement a lower limit as well, but in the last CL you mentioned that there is a new implementation of these and porting this logic might become difficult. So, kept this simple. > I think there is extra stuff to handling the labeling, i see the following for > Sqlite metrics? Maybe ask isherman about how to handle the suffixes? > > <histogram_suffixes name="SqliteDatabases" separator="."> > <mailto:owner>shess@chromium.org</owner> > <suffix name="Activity" label="Activity"/> > <suffix name="Affiliation" label="Affiliation"/> > <suffix name="AppCache" label="AppCache"/> > <suffix name="BookmarkImages" label="BookmarkImages"/> > <suffix name="Cookie" label="Cookie"/> > <suffix name="DatabaseTracker" label="DatabaseTracker"/> > <suffix name="DomainBoundCerts" label="DomainBoundCerts"/> > <suffix name="DOMStorageDatabase" label="DOMStorageDatabase"/> > <suffix name="History" label="History"/> > <suffix name="OfflinePageMetadata" label="OfflinePageMetadata"/> > <suffix name="Passwords" label="Passwords"/> > <suffix name="Predictor" label="Predictor"/> > <suffix name="Quota" label="Quota"/> > <suffix name="Shortcuts" label="Shortcuts"/> > <suffix name="SyncDirectory" label="SyncDirectory"/> > <suffix name="Text" label="Text (obsolete 7/24/13)"/> > <suffix name="Thumbnail" label="Thumbnail"/> > <suffix name="TopSites" label="TopSites"/> > <suffix name="Web" label="Web"/> > <affected-histogram name="Sqlite.AutoCommitTime"/> > <affected-histogram name="Sqlite.CommitTime"/> > <affected-histogram name="Sqlite.Error"/> > <affected-histogram name="Sqlite.QueryTime"/> > <affected-histogram name="Sqlite.SizeKB"/> > <affected-histogram name="Sqlite.Stats"/> > <affected-histogram name="Sqlite.UpdateTime"/> > <affected-histogram name="Sqlite.Version"/> > </histogram_suffixes> Thanks for pointing out. I have made changes and will add isherman to check this. > Ok, I think the if else conditions need rearranging to see the full set, > otherwise the pair of exceeded reasons will never get logged for low end > devices. That and the histogram.xml changes and this looks good. Makes sense. Fixed the order. Changed histograms.xml as well. > LocalStorage.BrowserLocalStorageCachePurgedInKB.InactiveOnLowEndDevice > LocalStorage.BrowserLocalStorageCachePurgedInKB.SizeLimitExceeded > LocalStorage.BrowserLocalStorageCachePurgedInKB.AreaCountLimitExceeded > LocalStorage.BrowserLocalStorageCachePurgedInKB.MemoryPressureSignal I have also added the memory pressure level indicator in these levels. > Given the finer grained stats, do we still want the general > LocalStorage.BrowserLocalStorageCachePurgedInKB that gets logged regardless of > the reason? Still think it is good to have one total goto metric to see if this logic is performing well. I was thinking the other metrics can be removed once we make sure we get desired results from the total one.
ssid@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for histograms.xml change. michaeln@: isherman seems to be OOO.
histograms.xml lgtm
lgtm 2
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2073523003/#ps40001 (title: "Re-arrange ifs and add histogram names.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073523003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add tagged histogram with reasons of purging the dom storage databases The existing metric shows us useful information on how much cache is purged. But, we do not know about the reason for purging. Since there a lot of cases where we are purging less than 10KB, it would be better to understand this case and try to avoid triggering purge code. BUG= ========== to ========== Add tagged histogram with reasons of purging the dom storage databases The existing metric shows us useful information on how much cache is purged. But, we do not know about the reason for purging. Since there a lot of cases where we are purging less than 10KB, it would be better to understand this case and try to avoid triggering purge code. BUG= Committed: https://crrev.com/cc032854d31ede369b1cfb2090ea3abf55538630 Cr-Commit-Position: refs/heads/master@{#400847} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc032854d31ede369b1cfb2090ea3abf55538630 Cr-Commit-Position: refs/heads/master@{#400847}
Message was sent while issue was closed.
Description was changed from ========== Add tagged histogram with reasons of purging the dom storage databases The existing metric shows us useful information on how much cache is purged. But, we do not know about the reason for purging. Since there a lot of cases where we are purging less than 10KB, it would be better to understand this case and try to avoid triggering purge code. BUG= Committed: https://crrev.com/cc032854d31ede369b1cfb2090ea3abf55538630 Cr-Commit-Position: refs/heads/master@{#400847} ========== to ========== Add tagged histogram with reasons of purging the dom storage databases The existing metric shows us useful information on how much cache is purged. But, we do not know about the reason for purging. Since there a lot of cases where we are purging less than 10KB, it would be better to understand this case and try to avoid triggering purge code. BUG=610551 Committed: https://crrev.com/cc032854d31ede369b1cfb2090ea3abf55538630 Cr-Commit-Position: refs/heads/master@{#400847} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
