Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2965083002:
Reduce Histogram object size. (Closed)
|
Created:
3 years, 5 months ago by bcwhite Modified:
3 years, 5 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReduce Histogram object size.
1) Remove |final_delta_created_| on non-DCHECK builds. Though this is
only a "bool", it's effectively a native int due to object size rounding
requirements.
2) Remove |declared_min_| and |declared_max_| fields which are used
only for checks and serialization, the values of which can be determined
from the range buckets.
These remove 16 bytes from an 88-byte structure on 64-bit, or about 20%.
BUG=733380
Review-Url: https://codereview.chromium.org/2965083002
Cr-Commit-Position: refs/heads/master@{#484585}
Committed: https://chromium.googlesource.com/chromium/src/+/f1dec483900f65168b0603556c1cb77c2f634864
Patch Set 1 #Patch Set 2 : put #if around DCHECKs of final_delta_created because macros still 'use' the vars #
Total comments: 3
Patch Set 3 : added TODO to remove unnecessary parameters #
Messages
Total messages: 24 (17 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reduce Histogram object size. 1) Remove |final_delta_created_| on non-DCHECK builds. Though this is only a "bool", it's effectively a native int due to object size rounding requirements. 2) Remove |declared_min_| and |declared_max_| fields which are used only for checks and serialization, the values of which can be determined from the range buckets. BUG=733380 ========== to ========== Reduce Histogram object size. 1) Remove |final_delta_created_| on non-DCHECK builds. Though this is only a "bool", it's effectively a native int due to object size rounding requirements. 2) Remove |declared_min_| and |declared_max_| fields which are used only for checks and serialization, the values of which can be determined from the range buckets. These remove 16 bytes from an 88-byte structure on 64-bit, or about 20%. BUG=733380 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm nice! https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.... base/metrics/histogram.cc:543: Sample maximum, Can we remove these params as well? I see it's only used in the CHECK below now. Maybe expand the TODO to also remove these params once that instrumentation is removed? Same for the function below.
https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.... base/metrics/histogram.cc:543: Sample maximum, On 2017/07/05 20:15:45, Alexei Svitkine (slow) wrote: > Can we remove these params as well? > > I see it's only used in the CHECK below now. Maybe expand the TODO to also > remove these params once that instrumentation is removed? > > Same for the function below. Possible. It would remove them as parameters from a fairly long call chain, too. declared_min() and declared_max() could be used in the log statements. Or we could keep them as a "sober second look" (i.e. DCHECK). What do you think?
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2965083002/diff/20001/base/metrics/histogram.... base/metrics/histogram.cc:543: Sample maximum, Going with the TODO for now.
The CQ bit was unchecked by bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2965083002/#ps40001 (title: "added TODO to remove unnecessary parameters")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499351169426430,
"parent_rev": "066741e326a180a2baf95aa69b317c7f468fbdb9", "commit_rev":
"f1dec483900f65168b0603556c1cb77c2f634864"}
Message was sent while issue was closed.
Description was changed from ========== Reduce Histogram object size. 1) Remove |final_delta_created_| on non-DCHECK builds. Though this is only a "bool", it's effectively a native int due to object size rounding requirements. 2) Remove |declared_min_| and |declared_max_| fields which are used only for checks and serialization, the values of which can be determined from the range buckets. These remove 16 bytes from an 88-byte structure on 64-bit, or about 20%. BUG=733380 ========== to ========== Reduce Histogram object size. 1) Remove |final_delta_created_| on non-DCHECK builds. Though this is only a "bool", it's effectively a native int due to object size rounding requirements. 2) Remove |declared_min_| and |declared_max_| fields which are used only for checks and serialization, the values of which can be determined from the range buckets. These remove 16 bytes from an 88-byte structure on 64-bit, or about 20%. BUG=733380 Review-Url: https://codereview.chromium.org/2965083002 Cr-Commit-Position: refs/heads/master@{#484585} Committed: https://chromium.googlesource.com/chromium/src/+/f1dec483900f65168b0603556c1c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f1dec483900f65168b0603556c1c...
Message was sent while issue was closed.
I think cleaning them up would be good - but TODO for this CL is definitely fine. On Thu, Jul 6, 2017 at 10:39 AM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/ > f1dec483900f65168b0603556c1cb77c2f634864 > > https://codereview.chromium.org/2965083002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
