Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674203003:
[memory-infra] Whitelist TabRestoreService MDP. (Closed)
|
Created:
3 years, 10 months ago by DmitrySkiba Modified:
3 years, 10 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[memory-infra] Whitelist TabRestoreService MDP.
This CL whitelists TabRestoreService MDP to make it available in
slow reports.
BUG=645123
Review-Url: https://codereview.chromium.org/2674203003
Cr-Commit-Position: refs/heads/master@{#449458}
Committed: https://chromium.googlesource.com/chromium/src/+/c72a640b0c237266f370c88c5f16e1fe30cbabb0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Don't create sizeless dumps #Patch Set 3 : Fix tests #
Messages
Total messages: 26 (11 generated)
dskiba@chromium.org changed reviewers: + primiano@chromium.org, ssid@chromium.org
The MDP runs on the main thread, but takes ~1.2ms on my Nexus 5X, see https://drive.google.com/file/d/0B_Hmi138MnbJSXk2cmFieHNkcTQ/view?usp=sharing
PTAL
https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:38: "TabRestoreServiceHelper", TabRestore service creates dumps even if there are no entries. Can we fix that before enabling on slow reports? We might have a column without size and I don't know what the memory mapper does in this case and might show weird results in dashboard. https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:129: "tab_restore/service_helper_0x?/entries/window_0x?", Is there a maximum number of service helpers and entries?
RS-LGTM once ssid is happy with timing and size of the trace (sorry backlogged cannot enter in the merit of this specific one) Unrelated: I have a feeling (from here and the other net discussion) that the entire EMU thing is showing some scalability limits and we should start looking for some alternate solution. I will try to reason on that problem tomorrow, have too many things open for today
RS-LGTM once ssid is happy with timing and size of the trace (sorry backlogged cannot enter in the merit of this specific one) Unrelated: I have a feeling (from here and the other net discussion) that the entire EMU thing is showing some scalability limits and we should start looking for some alternate solution. I will try to reason on that problem tomorrow, have too many things open for today
dskiba@chromium.org changed reviewers: + sky@chromium.org
sky@, please review small change in TabRestoreServiceHelper.
https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:38: "TabRestoreServiceHelper", On 2017/02/07 18:58:59, ssid wrote: > TabRestore service creates dumps even if there are no entries. Can we fix that > before enabling on slow reports? We might have a column without size and I don't > know what the memory mapper does in this case and might show weird results in > dashboard. Done. https://codereview.chromium.org/2674203003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:129: "tab_restore/service_helper_0x?/entries/window_0x?", On 2017/02/07 18:58:59, ssid wrote: > Is there a maximum number of service helpers and entries? Maximum value is 25, see TabRestoreServiceHelper::kMaxEntries
lgtm thanks.
LGTM
The CQ bit was checked by dskiba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2674203003/#ps20001 (title: "Don't create sizeless dumps")
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
Try jobs failed on following builders: linux_chromium_chromeos_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 dskiba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, sky@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2674203003/#ps40001 (title: "Fix tests")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dskiba@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": 40001, "attempt_start_ts": 1486667742365040,
"parent_rev": "1039b82bdad1ce37bd6716636e918f475ed50d13", "commit_rev":
"c72a640b0c237266f370c88c5f16e1fe30cbabb0"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Whitelist TabRestoreService MDP. This CL whitelists TabRestoreService MDP to make it available in slow reports. BUG=645123 ========== to ========== [memory-infra] Whitelist TabRestoreService MDP. This CL whitelists TabRestoreService MDP to make it available in slow reports. BUG=645123 Review-Url: https://codereview.chromium.org/2674203003 Cr-Commit-Position: refs/heads/master@{#449458} Committed: https://chromium.googlesource.com/chromium/src/+/c72a640b0c237266f370c88c5f16... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c72a640b0c237266f370c88c5f16... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
