Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450073002:
Include scheduling-only threads in tracks (Closed)
DescriptionInclude scheduling-only threads in tracks
If we have scheduling information for a thread,
include it visually in the track list. This makes
it easier to analyze a thread's behavior when
investigating system perf.
However to avoid information overload, put these
scheduling-only threads into a sub group in the
process that's collapsed by default.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ca8992d0320dc62890a697b91b977ccadf9bdb1e
Patch Set 1 #
Total comments: 4
Patch Set 2 : Include scheduling-only threads in tracks #
Total comments: 16
Patch Set 3 : Include scheduling-only threads in tracks #
Total comments: 10
Patch Set 4 : Include scheduling-only threads in tracks #
Total comments: 2
Patch Set 5 : Include scheduling-only threads in tracks #
Messages
Total messages: 21 (6 generated)
jreck@google.com changed reviewers: + ccraik@google.com, nduca@chromium.org
https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... tracing/tracing/ui/tracks/sched_group_track.html:1: <!DOCTYPE html> Seems like this will need to be duplicated to similarly collapse counters? https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... tracing/tracing/ui/tracks/sched_group_track.html:3: Copyright (c) 2013 The Chromium Authors. All rights reserved. 2016
Description was changed from ========== Include scheduling-only threads in tracks If we have scheduling information for a thread, include it visually in the track list. This makes it easier to analyze a thread's behavior when investigating system perf. However to avoid information overload, put these scheduling-only threads into a sub group in the process that's collapsed by default. BUG=catapult:# ========== to ========== Include scheduling-only threads in tracks If we have scheduling information for a thread, include it visually in the track list. This makes it easier to analyze a thread's behavior when investigating system perf. However to avoid information overload, put these scheduling-only threads into a sub group in the process that's collapsed by default. ==========
https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... tracing/tracing/ui/tracks/sched_group_track.html:1: <!DOCTYPE html> On 2016/10/25 22:05:36, Chris Craik wrote: > Seems like this will need to be duplicated to similarly collapse counters? Seems to be the way of things yeah https://codereview.chromium.org/2450073002/diff/1/tracing/tracing/ui/tracks/s... tracing/tracing/ui/tracks/sched_group_track.html:3: Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/10/25 22:05:36, Chris Craik wrote: > 2016 Done.
LGTM, but wait for nduca@
totally swamped. lgtm roughly. cc'ing people who can review after the fact in case you land but they see outrageous concerns that need fixing. :)
charliea@chromium.org changed reviewers: + charliea@chromium.org
(Taking a look now)
https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... File tracing/tracing/extras/importer/linux_perf/sched_parser.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/importer/linux_perf/sched_parser.html:71: if ('tgid' in eventBase && eventBase.tgid) { Is this different than just doing: if (eventBase.tgid !== undefined) ? https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/importer/linux_perf/sched_parser.html:72: var process = this.importer.model_.getOrCreateProcess(eventBase.tgid); nit (here and in rest of func): use a two space indent, not four https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/container_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/container_track.html:78: this.tracks_.forEach(function(track) { Any reason this is applicable to this CL? We're generally trying to move away from functional iteration (forEach) and towards ES6 style for...of iteration, but it seems like this should have no effect either way. If you think that we should be iterating through this.tracks_ instead of this.children (very possible), I'd recommend fixing that in a separate CL unless it's necessary for this one. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (left): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:140: addEventsToTrackMap: function(eventToTrackMap) { (Similar question about why this was deleted) https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:270: if (schedThreads.length > 0) { For a process like Chrome, how many of these schedThreads are we talking about? https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: 2016 Chromium copyright doesn't need (c) https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:19: * A track that groups tracks into a collapsable section. I think that this jsdoc could be improved. If it's called this, I'd imagine that it should be something like "Collapsible track" or something like that, but it looks like this is unique to Scheduler groups? In that case, I'd say that the description should have to do with scheduler groups and the fact that it's collapsible is probably just an implementation detail.
https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... File tracing/tracing/extras/importer/linux_perf/sched_parser.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/importer/linux_perf/sched_parser.html:71: if ('tgid' in eventBase && eventBase.tgid) { On 2016/10/26 16:48:05, charliea wrote: > Is this different than just doing: > > if (eventBase.tgid !== undefined) > > ? I guess not. I can never remember if it's legal to try to access a field that isn't ever set in JS or not :/ Done. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/importer/linux_perf/sched_parser.html:72: var process = this.importer.model_.getOrCreateProcess(eventBase.tgid); On 2016/10/26 16:48:05, charliea wrote: > nit (here and in rest of func): use a two space indent, not four Done. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/container_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/container_track.html:78: this.tracks_.forEach(function(track) { On 2016/10/26 16:48:05, charliea wrote: > Any reason this is applicable to this CL? We're generally trying to move away > from functional iteration (forEach) and towards ES6 style for...of iteration, > but it seems like this should have no effect either way. If you think that we > should be iterating through this.tracks_ instead of this.children (very > possible), I'd recommend fixing that in a separate CL unless it's necessary for > this one. It's necessary because otherwise this tries to call addEventsToTrackMap on the header that gets added by sched_group_track. It seemed cleaner to have this just iterate over tracks_ rather than have sched_group_track also override this method like process_track_base did since container_track already knows what's a track and what isn't and addEventsToTrackMap is only valid for tracks. So I moved the impl from process_track_base to here so that this "Just Worked" instead of requiring it to be overridden for any subclass that tried to have children that weren't all subclasses of track. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (left): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:140: addEventsToTrackMap: function(eventToTrackMap) { On 2016/10/26 16:48:05, charliea wrote: > (Similar question about why this was deleted) This impl was just moved to the subclass, so the override is no longer necessary since it does the same thing. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:270: if (schedThreads.length > 0) { On 2016/10/26 16:48:05, charliea wrote: > For a process like Chrome, how many of these schedThreads are we talking about? There's only a difference if ftrace tracing is on to get the sched_switch events which chrome://tracing doesn't seem to do normally. I guess you have to manually enable it or otherwise grant chrome access to /sys/kernel/debug if it's supported at all. But for Chrome I'm guessing not many if all the trace categories are selected as I'm guessing most threads will already have slices for them anyway, but it's potentially lots. The use case that drove this was a Unity game on Android and we wanted to see what its threads were doing more clearly, and it has ~30 threads with scheduling information but no slices. Thus, collapsed initially so there's minimal difference if ftracing is enabled, and no difference if it's not. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/10/26 16:48:05, charliea wrote: > nit: 2016 Chromium copyright doesn't need (c) Done. https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:19: * A track that groups tracks into a collapsable section. On 2016/10/26 16:48:05, charliea wrote: > I think that this jsdoc could be improved. If it's called this, I'd imagine that > it should be something like "Collapsible track" or something like that, but it > looks like this is unique to Scheduler groups? In that case, I'd say that the > description should have to do with scheduler groups and the fact that it's > collapsible is probably just an implementation detail. Oops, comment is out of date. I originally tried a generic collapsable track group with process_base putting ThreadTracks into it so I could re-use it elsewhere, but that doesn't seem possible to do with the binding model so I had to change it. Updated the comment.
https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/container_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/container_track.html:78: this.tracks_.forEach(function(track) { That makes sense. In that case, could you make this: for (var track of this.tracks_) { ... } https://codereview.chromium.org/2450073002/diff/40001/tracing/trace_viewer.gypi File tracing/trace_viewer.gypi (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/trace_viewer.gy... tracing/trace_viewer.gypi:488: 'tracing/ui/tracks/sched_group_track.html', I'm not sure that sched_group_track conveys the right idea here. All of the existing *_group_tracks (https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u...) help display entities called ______Group (e.g. ObjectInstanceGroup, SliceGroup, AsyncSliceGroup). In this case, we're instead displaying a group of threads that have nowhere else to live. Another problem is that this leaks some implementation details of ftrace into the view code: most people don't know what a sched is, or what a group of sched's might look like. I think that maybe something like other_thread_track.html might be a better name, although I still don't love it: I just can't think of anything better. Then, in that file, we describe that this is a place for "other" threads to live that don't have much information about them. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:276: if (schedThreads.length > 1 && hasVisibleThreads) { I think that we probably want to always add these threads to the SchedGroupTrack and add the ability to hide the expand/collapse bar for the track. My reasoning is that, right now, we only care about showing a simple ThreadTrack for each of these threads. If, in the future, we have some more custom logic (like shading an area where we're doing disk I/O in another color for example), we'd still want the stuff in the else() block of this branch to benefit from that, whereas now it wouldn't because it wouldn't be piped through the SchedGroupTrack where it makes sense for that logic to live. In other words, I think it makes sense to do something like: var track = new tr.ui.tracks.SchedGroupTrack(this.viewport); track.threads = schedThreads; track.collapsible = schedThreads.length > 1 && hasVisibleThreads; and then handle all of the rest of the logic in sched_group_thread https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:8: <link rel="import" href="/tracing/base/sorted_array_utils.html"> I don't think we use sorted_array_utils in here, right? https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:33: __proto__: tr.ui.tracks.ContainerTrack.prototype, I think that this should actually use MultiRowTrack (https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u...) as its prototype rather than just ContainerTrack: that should help provide a lot of the logic for collapsing. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:83: this.threads_.forEach(function(thread) { We're moving towards using for...of loops, so we should do that here: for (var thread of this.threads_) { ... }
https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/container_track.html (right): https://codereview.chromium.org/2450073002/diff/20001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/container_track.html:78: this.tracks_.forEach(function(track) { On 2016/10/26 21:23:37, charliea wrote: > That makes sense. In that case, could you make this: > > for (var track of this.tracks_) { > ... > } > Done. https://codereview.chromium.org/2450073002/diff/40001/tracing/trace_viewer.gypi File tracing/trace_viewer.gypi (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/trace_viewer.gy... tracing/trace_viewer.gypi:488: 'tracing/ui/tracks/sched_group_track.html', On 2016/10/26 21:23:37, charliea wrote: > I'm not sure that sched_group_track conveys the right idea here. > > All of the existing *_group_tracks > (https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u...) > help display entities called ______Group (e.g. ObjectInstanceGroup, SliceGroup, > AsyncSliceGroup). In this case, we're instead displaying a group of threads that > have nowhere else to live. > > Another problem is that this leaks some implementation details of ftrace into > the view code: most people don't know what a sched is, or what a group of > sched's might look like. > > I think that maybe something like other_thread_track.html might be a better > name, although I still don't love it: I just can't think of anything better. > Then, in that file, we describe that this is a place for "other" threads to live > that don't have much information about them. Done. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/process_track_base.html (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/process_track_base.html:276: if (schedThreads.length > 1 && hasVisibleThreads) { On 2016/10/26 21:23:37, charliea wrote: > I think that we probably want to always add these threads to the SchedGroupTrack > and add the ability to hide the expand/collapse bar for the track. > > My reasoning is that, right now, we only care about showing a simple ThreadTrack > for each of these threads. If, in the future, we have some more custom logic > (like shading an area where we're doing disk I/O in another color for example), > we'd still want the stuff in the else() block of this branch to benefit from > that, whereas now it wouldn't because it wouldn't be piped through the > SchedGroupTrack where it makes sense for that logic to live. > > In other words, I think it makes sense to do something like: > > var track = new tr.ui.tracks.SchedGroupTrack(this.viewport); > track.threads = schedThreads; > track.collapsible = schedThreads.length > 1 && hasVisibleThreads; > > and then handle all of the rest of the logic in sched_group_thread Done. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/sched_group_track.html (right): https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:8: <link rel="import" href="/tracing/base/sorted_array_utils.html"> On 2016/10/26 21:23:37, charliea wrote: > I don't think we use sorted_array_utils in here, right? Done. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:33: __proto__: tr.ui.tracks.ContainerTrack.prototype, On 2016/10/26 21:23:37, charliea wrote: > I think that this should actually use MultiRowTrack > (https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u...) > as its prototype rather than just ContainerTrack: that should help provide a lot > of the logic for collapsing. I originally tried MultiRowTrack but couldn't figure out how to get it to work. It seems to make assumptions that are not generically true. Also MultiRowTrack doesn't collapse to hidden it collapses to a smaller height which isn't desired here. https://codereview.chromium.org/2450073002/diff/40001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/sched_group_track.html:83: this.threads_.forEach(function(thread) { On 2016/10/26 21:23:37, charliea wrote: > We're moving towards using for...of loops, so we should do that here: > > for (var thread of this.threads_) { > ... > } Done.
lgtm https://codereview.chromium.org/2450073002/diff/60001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/other_threads_track.html (right): https://codereview.chromium.org/2450073002/diff/60001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/other_threads_track.html:99: }, nit: no need for extra comma or extra line following it
Thanks for the improvements John!
https://codereview.chromium.org/2450073002/diff/60001/tracing/tracing/ui/trac... File tracing/tracing/ui/tracks/other_threads_track.html (right): https://codereview.chromium.org/2450073002/diff/60001/tracing/tracing/ui/trac... tracing/tracing/ui/tracks/other_threads_track.html:99: }, On 2016/10/27 18:30:39, charliea wrote: > nit: no need for extra comma or extra line following it Done.
The CQ bit was checked by jreck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com, nduca@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2450073002/#ps80001 (title: "Include scheduling-only threads in tracks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include scheduling-only threads in tracks If we have scheduling information for a thread, include it visually in the track list. This makes it easier to analyze a thread's behavior when investigating system perf. However to avoid information overload, put these scheduling-only threads into a sub group in the process that's collapsed by default. ========== to ========== Include scheduling-only threads in tracks If we have scheduling information for a thread, include it visually in the track list. This makes it easier to analyze a thread's behavior when investigating system perf. However to avoid information overload, put these scheduling-only threads into a sub group in the process that's collapsed by default. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
