Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647353003:
Make LocalMemory (aka InMemory) the default for persistent histograms. (Closed)
|
Created:
3 years, 11 months ago by bcwhite Modified:
3 years, 10 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake LocalMemory (aka InMemory) the default for persistent histograms.
Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk.
IF THIS BREAKS SOMETHING, it can be reverted but be aware that it is only enabling something by default that is already running at a 100% experiment in Canary, Dev, and Beta, plus 20% in stable. Usually, the problems are only with the reporting of information via histograms, not with whatever is under test.
BUG=583440
Review-Url: https://codereview.chromium.org/2647353003
Cr-Original-Commit-Position: refs/heads/master@{#445516}
Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35cc032556dfe6
Review-Url: https://codereview.chromium.org/2647353003
Cr-Commit-Position: refs/heads/master@{#448265}
Committed: https://chromium.googlesource.com/chromium/src/+/6ebe16e737195e35e4cb250dce734817bd3db339
Patch Set 1 #Patch Set 2 : better conditional #Patch Set 3 : rebased #Patch Set 4 : make LocalMemory be the default for now #Messages
Total messages: 31 (21 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...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm
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 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/2647353003/#ps20001 (title: "better conditional")
The CQ bit was checked by bcwhite@chromium.org
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": 20001, "attempt_start_ts": 1485207383779350,
"parent_rev": "f076d91e660f7289f0ee4bbda0cd1f89563760f3", "commit_rev":
"ba68aeb47c44cb7ccc9b1e1cab35cc032556dfe6"}
Message was sent while issue was closed.
Description was changed from ========== Make MappedFile (aka OnDisk) the default for persistent histograms. BUG=583440 ========== to ========== Make MappedFile (aka OnDisk) the default for persistent histograms. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35...
Message was sent while issue was closed.
IF YOUR TEST IS FAILING because of this CL then it's likely trying to read the contents of histograms set by sub-processes. Try calling SubprocessMetricsProvider::MergeHistogramDeltasForTesting() before checking histogram counts to see if that gets the results you want.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2649973007/ by warx@chromium.org. The reason for reverting is: This patch breaks the chrome-pfq build with errors: https://bugs.chromium.org/p/chromium/issues/detail?id=674209, a sample auto-bug is: https://bugs.chromium.org/p/chromium/issues/detail?id=684212.
Message was sent while issue was closed.
Description was changed from ========== Make MappedFile (aka OnDisk) the default for persistent histograms. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ========== to ========== Make MappedFile (aka OnDisk) the default for persistent histograms. IF YOUR TEST IS FAILING because of this CL then it's likely trying to read the contents of histograms set by sub-processes. Try calling SubprocessMetricsProvider::MergeHistogramDeltasForTesting() before checking histogram counts to see if that gets the results you want. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ==========
Description was changed from ========== Make MappedFile (aka OnDisk) the default for persistent histograms. IF YOUR TEST IS FAILING because of this CL then it's likely trying to read the contents of histograms set by sub-processes. Try calling SubprocessMetricsProvider::MergeHistogramDeltasForTesting() before checking histogram counts to see if that gets the results you want. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ========== to ========== Make LocalMemory (aka InMemory) the default for persistent histograms. Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ==========
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...
warx has confirmed that the auto-merging of subprocess histograms means that this CL will not affect his build so I'd like to re-land it. I've change the default to "InMemory" in order to do this transition in two steps. This will allow us to determine if any regressions are due to the new design or due to being backed by disk.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Make LocalMemory (aka InMemory) the default for persistent histograms. Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ========== to ========== Make LocalMemory (aka InMemory) the default for persistent histograms. Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk. IF THIS BREAKS SOMETHING, it can be reverted but be aware that it is only enabling something by default that is already running at a 100% experiment in Canary, Dev, and Beta, plus 20% in stable. Usually, the problems are only with the reporting of information via histograms, not with whatever is under test. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ==========
The CQ bit was checked by bcwhite@chromium.org
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": 60001, "attempt_start_ts": 1486394543385380,
"parent_rev": "5a9feb1d8bd7fbd10bd4bfbb503cca7f4aee6e9d", "commit_rev":
"6ebe16e737195e35e4cb250dce734817bd3db339"}
Message was sent while issue was closed.
Description was changed from ========== Make LocalMemory (aka InMemory) the default for persistent histograms. Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk. IF THIS BREAKS SOMETHING, it can be reverted but be aware that it is only enabling something by default that is already running at a 100% experiment in Canary, Dev, and Beta, plus 20% in stable. Usually, the problems are only with the reporting of information via histograms, not with whatever is under test. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... ========== to ========== Make LocalMemory (aka InMemory) the default for persistent histograms. Use only shared memory as the default and change this to file-backed memory later so that it'll be possible to tell if any regressions occur due to the design or due to the disk. IF THIS BREAKS SOMETHING, it can be reverted but be aware that it is only enabling something by default that is already running at a 100% experiment in Canary, Dev, and Beta, plus 20% in stable. Usually, the problems are only with the reporting of information via histograms, not with whatever is under test. BUG=583440 Review-Url: https://codereview.chromium.org/2647353003 Cr-Original-Commit-Position: refs/heads/master@{#445516} Committed: https://chromium.googlesource.com/chromium/src/+/ba68aeb47c44cb7ccc9b1e1cab35... Review-Url: https://codereview.chromium.org/2647353003 Cr-Commit-Position: refs/heads/master@{#448265} Committed: https://chromium.googlesource.com/chromium/src/+/6ebe16e737195e35e4cb250dce73... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6ebe16e737195e35e4cb250dce73... |
