Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 196383016: Put histogram code in disk_cache on a diet. (Closed)

Created:
6 years, 9 months ago by Daniel Bratell
Modified:
6 years, 9 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, mmenke, asanka
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Put histogram code in disk_cache on a diet. The CACHE_UMA macro expanded wildly and with all the static variables in all blocks it was impossible for the compiler to shrink it. This removes the static variables (for a minor performance hit) to save 75% of the size of BackendImpl::ReportStats() and other changes elsewhere. Examples (sorry, don't have data for symbols smaller than 6 KB so not exact): disk_cache::BackendImpl::ReportStats() 55423 bytes -> 9788 bytes disk_cache::BackendImpl::FirstEviction() 21361 bytes -> < 6 KB disk_cache::BackendImpl::OpenEntryImpl: 16381 bytes -> < 6 KB disk_cache::EntryImpl::ReportIOTime: 14626 bytes -> < 6 KB disk_cache::Eviction::ReportListStats: 10099 bytes -> < 6 KB disk_cache::Eviction::TrimCacheV2: 7820 bytes -> < 6 KB disk_cache::Eviction::TrimCache: 7095 bytes -> < 6 KB In total my stripped Linux x64 Release content_shell binary shrinks by 132 KB with this change (0.25%). BUG=352555 R=gavinp@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257637

Patch Set 1 #

Total comments: 9

Patch Set 2 : Same change also in v3 #

Total comments: 1

Patch Set 3 : Fewer comment lines. #

Patch Set 4 : Restored switches with assert. #

Total comments: 1

Patch Set 5 : Restored more code. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -37 lines) Patch
M net/disk_cache/blockfile/histogram_macros.h View 1 2 3 4 chunks +7 lines, -24 lines 0 comments Download
M net/disk_cache/blockfile/histogram_macros_v3.h View 1 2 3 4 4 chunks +3 lines, -13 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
Daniel Bratell
mmenke, can you please review this or forward to someone else if you are not ...
6 years, 9 months ago (2014-03-14 10:16:16 UTC) #1
mmenke
Redirecting to gavinp.
6 years, 9 months ago (2014-03-14 12:50:23 UTC) #2
rvargas (doing something else)
https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h File net/disk_cache/blockfile/histogram_macros.h (right): https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h#newcode26 net/disk_cache/blockfile/histogram_macros.h:26: base::HistogramBase* counter = base::Histogram::FactoryGet( \ Does this mean that ...
6 years, 9 months ago (2014-03-14 21:48:41 UTC) #3
Daniel Bratell
https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h File net/disk_cache/blockfile/histogram_macros.h (right): https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h#newcode26 net/disk_cache/blockfile/histogram_macros.h:26: base::HistogramBase* counter = base::Histogram::FactoryGet( \ On 2014/03/14 21:48:41, rvargas ...
6 years, 9 months ago (2014-03-17 09:46:57 UTC) #4
gavinp
Daniel, This looks really good. I think histogram_macros_v3.h in the same directory probably needs to ...
6 years, 9 months ago (2014-03-17 10:15:27 UTC) #5
Daniel Bratell
Uploaded a new version where the same change was done to the v3 code and ...
6 years, 9 months ago (2014-03-17 14:56:04 UTC) #6
gavinp
This change LGTM, with or without my last nit. Thanks for looking into this and ...
6 years, 9 months ago (2014-03-17 15:06:28 UTC) #7
Daniel Bratell
The CQ bit was checked by bratell@opera.com
6 years, 9 months ago (2014-03-17 16:42:50 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/196383016/40001
6 years, 9 months ago (2014-03-17 16:42:55 UTC) #9
rvargas (doing something else)
https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h File net/disk_cache/blockfile/histogram_macros.h (right): https://codereview.chromium.org/196383016/diff/1/net/disk_cache/blockfile/histogram_macros.h#newcode26 net/disk_cache/blockfile/histogram_macros.h:26: base::HistogramBase* counter = base::Histogram::FactoryGet( \ On 2014/03/17 10:15:28, gavinp ...
6 years, 9 months ago (2014-03-17 18:03:08 UTC) #10
rvargas (doing something else)
The CQ bit was unchecked by rvargas@chromium.org
6 years, 9 months ago (2014-03-17 18:03:37 UTC) #11
rvargas (doing something else)
I forgot to mention that this looks fine as long as the plan is to ...
6 years, 9 months ago (2014-03-17 18:05:21 UTC) #12
Daniel Bratell
I opened http://code.google.com/p/chromium/issues/detail?id=353244 about looking at the default macros. Personally I wouldn't mind dropping static ...
6 years, 9 months ago (2014-03-17 19:35:53 UTC) #13
rvargas (doing something else)
On 2014/03/17 19:35:53, Daniel Bratell wrote: > I opened http://code.google.com/p/chromium/issues/detail?id=353244 about looking > at the ...
6 years, 9 months ago (2014-03-17 19:40:18 UTC) #14
rvargas (doing something else)
LGTM after fixing nit. https://codereview.chromium.org/196383016/diff/60001/net/disk_cache/blockfile/histogram_macros_v3.h File net/disk_cache/blockfile/histogram_macros_v3.h (right): https://codereview.chromium.org/196383016/diff/60001/net/disk_cache/blockfile/histogram_macros_v3.h#newcode98 net/disk_cache/blockfile/histogram_macros_v3.h:98: NOTREACHED();\ Don't add a NOTREACHED ...
6 years, 9 months ago (2014-03-17 19:40:59 UTC) #15
Daniel Bratell
On 2014/03/17 19:40:59, rvargas wrote: > LGTM after fixing nit. > > https://codereview.chromium.org/196383016/diff/60001/net/disk_cache/blockfile/histogram_macros_v3.h > File ...
6 years, 9 months ago (2014-03-17 19:57:06 UTC) #16
rvargas (doing something else)
On 2014/03/17 19:57:06, Daniel Bratell wrote: > On 2014/03/17 19:40:59, rvargas wrote: > > LGTM ...
6 years, 9 months ago (2014-03-17 20:37:35 UTC) #17
Daniel Bratell
The CQ bit was checked by bratell@opera.com
6 years, 9 months ago (2014-03-18 09:54:13 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/196383016/80001
6 years, 9 months ago (2014-03-18 09:54:18 UTC) #19
commit-bot: I haz the power
6 years, 9 months ago (2014-03-18 15:27:41 UTC) #20
Message was sent while issue was closed.
Change committed as 257637

Powered by Google App Engine
This is Rietveld 408576698