|
|
Created:
3 years, 9 months ago by manzagop (departed) Modified:
3 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPostmortem report collection: validate internal state
This change introduces checking of the underlying allocator's state
(initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit but instead marked as deleted.
BUG=691595
Review-Url: https://codereview.chromium.org/2767193002
Cr-Commit-Position: refs/heads/master@{#466561}
Committed: https://chromium.googlesource.com/chromium/src/+/0793c6c441b24c31bf2c890d243b1875a51abe80
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments #Patch Set 3 : Merge #Patch Set 4 : Introduce MarkDeleted on tracker #Patch Set 5 : Address comments #Patch Set 6 : Compile fixup #
Total comments: 2
Patch Set 7 : Address comment #Messages
Total messages: 46 (30 generated)
manzagop@chromium.org changed reviewers: + bcwhite@chromium.org
Hi Brian, This adds some allocator validity checks. Please have a look. Thanks! Pierre
https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:166: const PersistentMemoryAllocator& allocator() { return *allocator_; } For various historical reasons, allocators are always pointers rather than references. return allocator_.get(); But I would avoid this if possible as there may come a day when its not a single allocator but many. I can envision that if this moves to Renderers and security doesn't want them sharing a file with the Browser process. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); I think it would be better to add a GetMemoryState() method to GlobalActivityAnalyzer. It could then be smart about what it returns, especially in the face of multiple files under analysis. The analyzer's version could include other values: MEMORY_FULL = 99, MEMORY_CORRUPT = 98, etc. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:225: UMA_HISTOGRAM_BOOLEAN("ActivityTracker.Collect.IsAllocatorFull", I think it would be better to just enable the internal histograms of the allocator behind the tracker. I'll add that to the tracker. This way you'll know how much of the space is being used, rather than just after-the-fact that it became full.
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); > MEMORY_FULL = 99, > MEMORY_CORRUPT = 98, Make that... MEMORY_FULL = 100, // MEMORY_USER_DEFINED + 0 MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1
Description was changed from ========== Postmortem: validate allocator memory state, corruption and fullness BUG=691595 ========== to ========== Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit. BUG=691595 ==========
The CQ bit was checked by manzagop@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...
Patchset #2 (id:20001) has been deleted
Thanks! Another look? https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:166: const PersistentMemoryAllocator& allocator() { return *allocator_; } On 2017/03/23 11:32:44, bcwhite wrote: > For various historical reasons, allocators are always pointers rather than > references. > > return allocator_.get(); > > But I would avoid this if possible as there may come a day when its not a single > allocator but many. I can envision that if this moves to Renderers and > security doesn't want them sharing a file with the Browser process. Removing this, as per your other comments. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 11:35:40, bcwhite wrote: > > MEMORY_FULL = 99, > > MEMORY_CORRUPT = 98, > > Make that... > > MEMORY_FULL = 100, // MEMORY_USER_DEFINED + 0 > MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1 Hm, maybe it's best to decouple the two. Tell me what you think. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 11:32:44, bcwhite wrote: > I think it would be better to add a GetMemoryState() method to > GlobalActivityAnalyzer. It could then be smart about what it returns, > especially in the face of multiple files under analysis. > > The analyzer's version could include other values: > MEMORY_FULL = 99, > MEMORY_CORRUPT = 98, > etc. Acknowledged. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:225: UMA_HISTOGRAM_BOOLEAN("ActivityTracker.Collect.IsAllocatorFull", On 2017/03/23 11:32:44, bcwhite wrote: > I think it would be better to just enable the internal histograms of the > allocator behind the tracker. I'll add that to the tracker. > > This way you'll know how much of the space is being used, rather than just > after-the-fact that it became full. > Removed the fullness histogram. Will the histograms be specific to activity tracking?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 19:18:54, manzagop wrote: > On 2017/03/23 11:35:40, bcwhite wrote: > > > MEMORY_FULL = 99, > > > MEMORY_CORRUPT = 98, > > > > Make that... > > > > MEMORY_FULL = 100, // MEMORY_USER_DEFINED + 0 > > MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1 > > Hm, maybe it's best to decouple the two. Tell me what you think. The memory state was written expecting that it would be extended if a higher-level knew more detail so I think it's fine and better than creating something new that simply copies most of it and extends. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:225: UMA_HISTOGRAM_BOOLEAN("ActivityTracker.Collect.IsAllocatorFull", On 2017/03/23 19:18:54, manzagop wrote: > On 2017/03/23 11:32:44, bcwhite wrote: > > I think it would be better to just enable the internal histograms of the > > allocator behind the tracker. I'll add that to the tracker. > > > > This way you'll know how much of the space is being used, rather than just > > after-the-fact that it became full. > > > > Removed the fullness histogram. Will the histograms be specific to activity > tracking? They're specific to the allocator's name: UMA.PersistentAllocator.NAME.UsedPct
Thanks! Some replies. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 21:10:43, bcwhite wrote: > On 2017/03/23 19:18:54, manzagop wrote: > > On 2017/03/23 11:35:40, bcwhite wrote: > > > > MEMORY_FULL = 99, > > > > MEMORY_CORRUPT = 98, > > > > > > Make that... > > > > > > MEMORY_FULL = 100, // MEMORY_USER_DEFINED + 0 > > > MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1 > > > > Hm, maybe it's best to decouple the two. Tell me what you think. > > The memory state was written expecting that it would be extended if a > higher-level knew more detail so I think it's fine and better than creating > something new that simply copies most of it and extends. I'm not sure I follow. Here are the main things: - full/corrupt are properties of the allocator - why should they be in the user defined range? - if corrupt/full are valid memory states, should GetMemoryState return these? Should IsCorrupt/IsFull become private? - If we plan on having multiple files to analyze, then I think it makes sense to have a different enum to represent the overall state. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:225: UMA_HISTOGRAM_BOOLEAN("ActivityTracker.Collect.IsAllocatorFull", On 2017/03/23 21:10:43, bcwhite wrote: > On 2017/03/23 19:18:54, manzagop wrote: > > On 2017/03/23 11:32:44, bcwhite wrote: > > > I think it would be better to just enable the internal histograms of the > > > allocator behind the tracker. I'll add that to the tracker. > > > > > > This way you'll know how much of the space is being used, rather than just > > > after-the-fact that it became full. > > > > > > > Removed the fullness histogram. Will the histograms be specific to activity > > tracking? > > They're specific to the allocator's name: > > UMA.PersistentAllocator.NAME.UsedPct Great!
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); > > > > > MEMORY_FULL = 99, > > > > > MEMORY_CORRUPT = 98, > > > > > > > > Make that... > > > > > > > > MEMORY_FULL = 100, // MEMORY_USER_DEFINED + 0 > > > > MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1 > > > > > > Hm, maybe it's best to decouple the two. Tell me what you think. > > > > The memory state was written expecting that it would be extended if a > > higher-level knew more detail so I think it's fine and better than creating > > something new that simply copies most of it and extends. > > I'm not sure I follow. Here are the main things: > - full/corrupt are properties of the allocator - why should they be in the user > defined range? If you ignore the implementation details, you can simply think of it as the tracker's state. More accurately, full & corrupt are "flags" but since you're not reporting them that way upstream, I don't think it's important to differentiate them as such here, especially when there is a fairly clear priority order: full means nothing if corrupt, full and corrupt mean nothing if deleted, etc. > - if corrupt/full are valid memory states, should GetMemoryState return these? Yes. > Should IsCorrupt/IsFull become private? They allocator is private so those methods are inaccessible outside the analyzer. They're important to the user of the allocator (the analyzer in this case) but shouldn't be important to the user of the analyzer. > - If we plan on having multiple files to analyze, then I think it makes sense to > have a different enum to represent the overall state. Eventually I think it will go that way but what you'd really want is state per process. I'd hold off on that until it proved important in the field, however, as its likely to be rare enough to not warrant the effort. Accessing the allocator for state information in order to do analysis shouldn't be necessary since this is the "analyzer" class. This class should do the interpretation of the underlying data and report it in a meaningful manner to the caller. The only reason to access the allocator or any of its methods should be if non-tracker information was also being stored there for convenience purposes.
Description was changed from ========== Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit. BUG=691595 ========== to ========== Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit but instead marked as deleted. BUG=691595 ==========
I agree the clients of the tracker/analyzer shouldn't deal with the allocator. I've introduced a MarDeleted function that helps with this. Now I'm still not convinced about putting full/corrupt in the memory state, as that can lead to inconsistent allocator state with the flags. I still prefer introducing the TrackerState that is interpreted from the tracker's memory state + tracker's flags. If you disagree, let's sync live.
The CQ bit was checked by manzagop@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: This issue passed the CQ dry run.
Patchset #5 (id:100001) has been deleted
Ok, I've switched things as discussed so most of the allocator state checking happens in CreateWithFile. Please have another look. Thanks!
The CQ bit was checked by manzagop@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by manzagop@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_an... base/debug/activity_analyzer.cc:30: kInvalidMemoryMappedFile = 0, It's standard practice not to specify the numerical values. Noting that it's only okay to append (before "max") is sufficient.
The CQ bit was checked by manzagop@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...
Thanks! https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_an... base/debug/activity_analyzer.cc:30: kInvalidMemoryMappedFile = 0, On 2017/04/19 18:54:07, bcwhite wrote: > It's standard practice not to specify the numerical values. Noting that it's > only okay to append (before "max") is sufficient. Done.
manzagop@chromium.org changed reviewers: + holte@chromium.org
Hi Steven! Could you have an OWNERS' look at: chrome\browser\metrics\chrome_metrics_service_client.cc tools\metrics\histograms\histograms.xml Thanks! Pierre
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks for the review!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org Link to the patchset: https://codereview.chromium.org/2767193002/#ps160001 (title: "Address comment")
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": 160001, "attempt_start_ts": 1492945455725270, "parent_rev": "7df9336519376dca2df90eb4b1f975723d99fa6c", "commit_rev": "0793c6c441b24c31bf2c890d243b1875a51abe80"}
Message was sent while issue was closed.
Description was changed from ========== Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit but instead marked as deleted. BUG=691595 ========== to ========== Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit but instead marked as deleted. BUG=691595 Review-Url: https://codereview.chromium.org/2767193002 Cr-Commit-Position: refs/heads/master@{#466561} Committed: https://chromium.googlesource.com/chromium/src/+/0793c6c441b24c31bf2c890d243b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0793c6c441b24c31bf2c890d243b... |