Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797883002:
cc: Add TileManager::GetActivationStateAsValue for debugging state. (Closed)
Descriptioncc: Add TileManager::GetActivationStateAsValue for debugging state.
This patch adds a debugging function which can be used to retrieve
the current state of tile manager. Specifically, it logs some general
information like tree priority and memory use/limits. As well, it logs
the current raster queue and required for activation queue to see if
there is a mismatch between what is being rasterized vs what is being
considered for activation.
R=sunnyps@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2797883002
Cr-Commit-Position: refs/heads/master@{#462650}
Committed: https://chromium.googlesource.com/chromium/src/+/46076c58c1198abaac97db355103069c6ab2d2b2
Patch Set 1 #
Total comments: 2
Patch Set 2 : update #
Total comments: 4
Patch Set 3 : update #Patch Set 4 : compilefix #
Messages
Total messages: 22 (12 generated)
Description was changed from ========== cc: Add TileManager::GetActivationStateAsValue for debugging state. This patch adds a debugging function which can be used to retrieve the current state of tile manager. Specifically, it logs some general information like tree priority and memory use/limits. As well, it logs the current raster queue and required for activation queue to see if there is a mismatch between what is being rasterized vs what is being considered for activation. R=sunnyps@chromium.org ========== to ========== cc: Add TileManager::GetActivationStateAsValue for debugging state. This patch adds a debugging function which can be used to retrieve the current state of tile manager. Specifically, it logs some general information like tree priority and memory use/limits. As well, it logs the current raster queue and required for activation queue to see if there is a mismatch between what is being rasterized vs what is being considered for activation. R=sunnyps@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Please take a look to see if there's anything else you'd like to be dumped as
the state. I think this should be sufficient for us to track down the hang bug
if we dump this state at the right time. Here's a sample output (when passed to
ostringstream):
{
"activation_tiles": [ {
"content_rect": "0,0 256x256",
"contents_scale": 1.0,
"distance_to_visible": 0.0,
"id": 69,
"is_ready_to_draw": false,
"priority_bin": "NOW",
"required_for_activation": true,
"required_for_draw": false,
"resolution": "HIGH_RESOLUTION"
}, {
"content_rect": "254,0 256x256",
"contents_scale": 1.0,
"distance_to_visible": 0.0,
"id": 70,
"is_ready_to_draw": false,
"priority_bin": "NOW",
"required_for_activation": true,
"required_for_draw": false,
"resolution": "HIGH_RESOLUTION"
} ],
"current_memory_usage": 2228224,
"current_resource_usage": 9,
"hard_memory_limit": 536870912,
"pending_required_for_activation_callback_id": 0,
"raster_tiles": [ {
"content_rect": "0,0 256x256",
"contents_scale": 1.0,
"distance_to_visible": 0.0,
"id": 69,
"is_ready_to_draw": false,
"priority_bin": "NOW",
"required_for_activation": true,
"required_for_draw": false,
"resolution": "HIGH_RESOLUTION"
}, {
"content_rect": "254,0 256x256",
"contents_scale": 1.0,
"distance_to_visible": 0.0,
"id": 70,
"is_ready_to_draw": false,
"priority_bin": "NOW",
"required_for_activation": true,
"required_for_draw": false,
"resolution": "HIGH_RESOLUTION"
} ],
"soft_memory_limit": 536870912,
"tree_priority": "SAME_PRIORITY_FOR_BOTH_TREES"
}
I'm also not sure if we want to shorten some of these strings so that the whole
log is short enough. The example above is ~1488 bytes.
https://codereview.chromium.org/2797883002/diff/1/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2797883002/diff/1/cc/tiles/tile_manager.h#new... cc/tiles/tile_manager.h:234: void GetActivationStateAsValue(base::DictionaryValue* value); Can you make this return an std::unique_ptr<base::trace_event::ConvertableToTraceFormat> here? See Scheduler::AsValue for example. That way we can also trace this in a disabled-by-default category.
https://codereview.chromium.org/2797883002/diff/1/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2797883002/diff/1/cc/tiles/tile_manager.h#new... cc/tiles/tile_manager.h:234: void GetActivationStateAsValue(base::DictionaryValue* value); On 2017/04/05 21:09:50, sunnyps wrote: > Can you make this return an > std::unique_ptr<base::trace_event::ConvertableToTraceFormat> here? See > Scheduler::AsValue for example. That way we can also trace this in a > disabled-by-default category. Hm.. I specifically decided not to do this, since I don't think this is a reasonable thing to trace. Maybe it's worthwhile though
The CQ bit was checked by vmpstr@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...
PTAL
lgtm % nits https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1379: base::trace_event::TracedValue* value) { nit: I'd expect the value param to be exclusive to this tile, that is, the Begin/EndDictionary should be outside this lambda. Or maybe rename value to tile_list or something like that. https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1385: value->SetString("content_rect", tile->content_rect().ToString().c_str()); nit: SetString takes a StringPiece so you don't need to call c_str here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1379: base::trace_event::TracedValue* value) { On 2017/04/06 01:12:37, sunnyps wrote: > nit: I'd expect the value param to be exclusive to this tile, that is, the > Begin/EndDictionary should be outside this lambda. Or maybe rename value to > tile_list or something like that. Done. https://codereview.chromium.org/2797883002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1385: value->SetString("content_rect", tile->content_rect().ToString().c_str()); On 2017/04/06 01:12:37, sunnyps wrote: > nit: SetString takes a StringPiece so you don't need to call c_str here. Done.
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/2797883002/#ps40001 (title: "update")
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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/2797883002/#ps60001 (title: "compilefix")
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": 1491511801208880,
"parent_rev": "701fa21b5ccf60143b103799f83cae7c51e6b5ee", "commit_rev":
"46076c58c1198abaac97db355103069c6ab2d2b2"}
Message was sent while issue was closed.
Description was changed from ========== cc: Add TileManager::GetActivationStateAsValue for debugging state. This patch adds a debugging function which can be used to retrieve the current state of tile manager. Specifically, it logs some general information like tree priority and memory use/limits. As well, it logs the current raster queue and required for activation queue to see if there is a mismatch between what is being rasterized vs what is being considered for activation. R=sunnyps@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Add TileManager::GetActivationStateAsValue for debugging state. This patch adds a debugging function which can be used to retrieve the current state of tile manager. Specifically, it logs some general information like tree priority and memory use/limits. As well, it logs the current raster queue and required for activation queue to see if there is a mismatch between what is being rasterized vs what is being considered for activation. R=sunnyps@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2797883002 Cr-Commit-Position: refs/heads/master@{#462650} Committed: https://chromium.googlesource.com/chromium/src/+/46076c58c1198abaac97db355103... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/46076c58c1198abaac97db355103... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
