CPU Usage Timeline - Thread Breakdown - Thread breakdown - Main Thread, Worker Threads, other threads - Main Thread work breakdown - Script, Layout, Style Resolution, Paint
Created attachment 362324 [details] [IMAGE] Light Mode
Created attachment 362325 [details] [IMAGE] Dark Mode
Created attachment 362328 [details] [PATCH] Proposed Fix
I have some high-level comments and questions. Breaking down stacked area graph into 4 separate area graphs doesn't seem to add a lot of value. What if we just show a legend for the stacked area graph? You're using a step graph in the timeline overview and an area graph below for the same data. Why wouldn't we use the step graph everywhere? This visualization takes a lot of screen space. Do we expect it to be used mostly with detached Web Inspector? (In reply to Joseph Pecoraro from comment #1) > Created attachment 362324 [details] > [IMAGE] Light Mode What is "rs=ACT90oEozu7A2L..." on the screenshot? What is "Other Threads"? What falls into this category? WebKit Threads and Other Threads have the same color. I think could use slightly different colors.
(In reply to Nikita Vasilyev from comment #4) > I have some high-level comments and questions. > > Breaking down stacked area graph into 4 separate area graphs doesn't seem to > add a lot of value. What if we just show a legend for the stacked area graph? Yep that is something I'm looking into. Some tooltip/legend when hovering over the graph. > You're using a step graph in the timeline overview and an area graph below > for the same data. Why wouldn't we use the step graph everywhere? I've been receiving feedback that people want to see a line graph everywhere. The benefits of the column graph are: • Better shows concrete samples being taken and the approximate time duration between them (500ms) The benefits of the line graph: • Clearer to see trends over long times I think both have value, which is why I went with both in this patch. I'm still experimenting with different UI combinations, but I still like this the best. > This visualization takes a lot of screen space. Do we expect it to be used > mostly with detached Web Inspector? Hmm, this isn't worse than the Memory Timeline. Maybe we should reduce the Circle chart size? > What is "rs=ACT90oEozu7A2L..." on the screenshot? In this case it is the name of the Web Worker resource, which on this Google Maps page has a rather cryptic name. We can use this name, or something near it, to link to the Worker resource if needed. Inspecting Web Inspector you may see "FormatterWorker.js" or "HeapSnapshotWorker.js" which are the names of our workers. Workers are rare, but when they show up are good to get the CPU usage specifically for these threads. The number of workers can be dynamic. So there can be 0-N graphs for the different workers, each with their unique name. > What is "Other Threads"? What falls into this category? If you run `/usr/bin/sample` on a WebContent process you'll see the various threads. In practice for WebKit this tends to be global dispatch queues, CoreAnimation / CA worker queues, media threads / queues, that show up in the Web Content Process as a result of lower level frameworks. > WebKit Threads and Other Threads have the same color. I think could use > slightly different colors. I'll consider it. I already have 2 shades of green and a third might work. My main worry is that if WebKit Threads and Other Threads both end up being ~5% then the two color sections will be so small that you'll just end up with the border colors of the sections instead of much color for the individual sections. If that is commonly the case (which I recall it was in Idle testing) then I'd rather a combined and visible section then two small mostly non-visible sections.
(In reply to Joseph Pecoraro from comment #5) > (In reply to Nikita Vasilyev from comment #4) > > Breaking down stacked area graph into 4 separate area graphs doesn't seem to add a lot of value. What if we just show a legend for the stacked area graph? > Yep that is something I'm looking into. Some tooltip/legend when hovering over the graph. Having separate graphs allows for more isolated debugging/understanding. If I have a worker that is ever increasingly using memory, it's much easier to recognize that if it's separate than if it's just an increasingly larger part of a (likely) more complex segmented graph. > > You're using a step graph in the timeline overview and an area graph below for the same data. Why wouldn't we use the step graph everywhere? > I've been receiving feedback that people want to see a line graph everywhere. > > The benefits of the column graph are: > > • Better shows concrete samples being taken and the approximate time duration between them (500ms) > > The benefits of the line graph: > > • Clearer to see trends over long times > > I think both have value, which is why I went with both in this patch. I'm still experimenting with different UI combinations, but I still like this the best. For the non-combined graphs (e.g. not "Total"), I think this has merit. For the combined graph, I think it would be effectively a repeat of the overview area, so I'm not in favor of changing it for that (if we changed the non-combined graphs, then I think we should just get rid of "Total"). Another point to make is that we have to somehow convey this idea that the actual "value" of any given point is represented by the actually captured CPU usage, which is only every 500ms. If I start a recording and a CPU usage measurement is taken at 500ms and 1000ms, any other records between 500ms-1000ms will be "bunched" into a single column. The area chart at least conveys the idea that these "points" are really just that: points (e.g. a measurement taken at a very specific time, not a sustained/maintained value). The column should _not_ convey the idea that the given CPU utilization was sustained/maintained over the width (e.g. time span) of the column, and if it does, we need to rework this so as to make sure that that isn't what's derived.
(In reply to Joseph Pecoraro from comment #5) > > This visualization takes a lot of screen space. Do we expect it to be used > > mostly with detached Web Inspector? > > Hmm, this isn't worse than the Memory Timeline. Maybe we should reduce the > Circle chart size? It seems a little wasteful space-wise to leave so much space on both sides of the overview pie-chart. I think Memory Timeline isn't particularly docked-friendly, but at least it has two pie-charts at the top. Right now, I don't have a solution here. > > > > What is "rs=ACT90oEozu7A2L..." on the screenshot? > > In this case it is the name of the Web Worker resource, which on this Google > Maps page has a rather cryptic name. We can use this name, or something near > it, to link to the Worker resource if needed. > > Inspecting Web Inspector you may see "FormatterWorker.js" or > "HeapSnapshotWorker.js" which are the names of our workers. > > Workers are rare, but when they show up are good to get the CPU usage > specifically for these threads. The number of workers can be dynamic. So > there can be 0-N graphs for the different workers, each with their unique > name. Perhaps we could you a subtitle. > > > > What is "Other Threads"? What falls into this category? > > If you run `/usr/bin/sample` on a WebContent process you'll see the various > threads. > > In practice for WebKit this tends to be global dispatch queues, > CoreAnimation / CA worker queues, media threads / queues, that show up in > the Web Content Process as a result of lower level frameworks. I expect that most developers don't know all this. However, I don't know how much of it we want to explain it in the UI.
View in context: https://bugs.webkit.org/attachment.cgi?id=362328&action=review > Source/WebInspectorUI/ChangeLog:4 > + Web Inspector: CPU Usage Timeline - Thread Breakdown > + https://bugs.webkit.org/show_bug.cgi?id=194788 Perhaps it's worth mentioning that thread breakdown UI should be enabled in experimental settings to use. > Source/WebInspectorUI/ChangeLog:63 > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers): > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers > div): > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers > div > .label): > + (.timeline-view.cpu > .content): Deleted. > + (.cpu-usage-view .line-chart > svg > path): Deleted. Nit: line charts usually have just lines without any background. What you're using looks like an area chart to me. https://duckduckgo.com/?q=line+chart&t=h_&iar=images&iax=images&ia=images https://duckduckgo.com/?q=area+chart&t=h_&iar=images&iax=images&ia=images > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:46 > + this._workers = null; We could use an empty array here. > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:58 > + if (!this._workers) > + this._workers = []; ...and avoid this check. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:126 > + this._chart.addColumnSet(x, height, w, [h1, h2, h3]); Nit: h1 and h2 aren't used inside of the else clause. We could be defined above this line. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:103 > +.timeline-view.cpu > .content > .overview > .divider { > + margin: 0 5px; > + > + --cpu-timeline-view-overview-divider-border-end: 1px solid var(--border-color); > +} > + > +body[dir=ltr] .timeline-view.cpu > .content > .overview > .divider { > + border-right: var(--cpu-timeline-view-overview-divider-border-end); > +} > + > +body[dir=rtl] .timeline-view.cpu > .content > .overview > .divider { > + border-left: var(--cpu-timeline-view-overview-divider-border-end); > +} Where's the divider in the UI? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:205 > + bottom: 0; Nit: I'd put `pointer-events: none` here instead of each marker. --- I'll continue review from here ---
> It seems a little wasteful space-wise to leave so much space on both sides > of the overview pie-chart. > I think Memory Timeline isn't particularly docked-friendly, but at least it > has two pie-charts at the top. > Right now, I don't have a solution here. This will get a 2nd chart at the top, we just haven't settled on one yet. > > > What is "Other Threads"? What falls into this category? > > > > If you run `/usr/bin/sample` on a WebContent process you'll see the various > > threads. > > > > In practice for WebKit this tends to be global dispatch queues, > > CoreAnimation / CA worker queues, media threads / queues, that show up in > > the Web Content Process as a result of lower level frameworks. > > I expect that most developers don't know all this. However, I don't know how > much of it we want to explain it in the UI. Yes, I think documentation (such as a blog post) will be the best resource here. Developers have ways of viewing all threads in a process (sample, spindump, sysdiagnose, instruments). Web Inspector should strive to relate any activity back to Web Content, which right now the only threads where that is possible are the Main Thread and Worker Threads.
(In reply to Nikita Vasilyev from comment #8) > View in context: > https://bugs.webkit.org/attachment.cgi?id=362328&action=review I'm not sure why I'm unable to view your comments in context.
Comment on attachment 362328 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362328&action=review Oops, let's try again. > Source/WebInspectorUI/ChangeLog:4 > + Web Inspector: CPU Usage Timeline - Thread Breakdown > + https://bugs.webkit.org/show_bug.cgi?id=194788 Perhaps it's worth mentioning that thread breakdown UI should be enabled in experimental settings to use. > Source/WebInspectorUI/ChangeLog:63 > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers): > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers > div): > + (.timeline-view.cpu :matches(.line-chart, .stacked-line-chart) .markers > div > .label): > + (.timeline-view.cpu > .content): Deleted. > + (.cpu-usage-view .line-chart > svg > path): Deleted. Nit: line charts usually have just lines without any background. What you're using looks like an area chart to me. https://duckduckgo.com/?q=line+chart&t=h_&iar=images&iax=images&ia=images https://duckduckgo.com/?q=area+chart&t=h_&iar=images&iax=images&ia=images > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:46 > + this._workers = null; We could use an empty array here. > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:58 > + if (!this._workers) > + this._workers = []; ...and avoid this check. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:126 > + this._chart.addColumnSet(x, height, w, [h1, h2, h3]); Nit: h1 and h2 aren't used inside of the else clause. We could be defined above this line. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:103 > +.timeline-view.cpu > .content > .overview > .divider { > + margin: 0 5px; > + > + --cpu-timeline-view-overview-divider-border-end: 1px solid var(--border-color); > +} > + > +body[dir=ltr] .timeline-view.cpu > .content > .overview > .divider { > + border-right: var(--cpu-timeline-view-overview-divider-border-end); > +} > + > +body[dir=rtl] .timeline-view.cpu > .content > .overview > .divider { > + border-left: var(--cpu-timeline-view-overview-divider-border-end); > +} Where's the divider in the UI? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:205 > + bottom: 0; Nit: I'd put `pointer-events: none` here instead of each marker.
Comment on attachment 362328 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362328&action=review > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:164 > + this._breakdownLegendElement.classList.add("legend"); Main Thread section with the pie chart and a legend is nearly identical to the ones in the Memory tool. Have you considered abstracting them out in a separate view? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219 > + const cpuUsageViewHeight = 150; // Keep this in sync with .cpu-usage-stacked-view > + const threadCPUUsageViewHeight = 65; // Keep this in sync with .cpu-usage-view Have you considered doing something like this? element.style.setProperty("--cpu-usage-height", cpuUsageViewHeight + "px") > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:406 > + function markerValuesForMaxValue(max) { > + if (max < 1) > + return [0.5]; > + if (max < 7) > + return removeGreaterThan([1, 3, 5], max); > + if (max < 12.5) > + return removeGreaterThan([5, 10], max); > + if (max < 20) > + return removeGreaterThan([5, 10, 15], max); > + if (max < 30) > + return removeGreaterThan([10, 20, 30], max); > + if (max < 50) > + return removeGreaterThan([15, 30, 45], max); > + if (max < 100) > + return removeGreaterThan([25, 50, 75], max); > + if (max < 200) > + return removeGreaterThan([50, 100, 150], max); > + if (max >= 200) { > + let hundreds = Math.floor(max / 100); > + let even = (hundreds % 2) === 0; > + if (even) { > + let top = hundreds * 100; > + let bottom = top / 2; > + return [bottom, top]; > + } > + let top = hundreds * 100; > + let bottom = 100; > + let mid = (top + bottom) / 2; > + return [bottom, mid, top]; > + } > + } Is it the first time we use this logic? Perhaps this could be moved to Utilities and have unit tests. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:29 > + height: 151px; /* Keep this in sync with cpuUsageViewHeight + 1 (for border-bottom) */ Consider defining CSS variables via JS. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:61 > + transform: scaleX(-1); This flips text labels as well, e.g. "50%". > Source/WebInspectorUI/UserInterface/Views/CPUUsageView.css:29 > + height: 66px; /* Keep this in sync with threadCPUUsageViewHeight + 1 (for border-bottom) */ Consider defining CSS variables via JS. > Source/WebInspectorUI/UserInterface/Views/LegacyCPUTimelineView.js:108 > + const cpuUsageViewHeight = 65; // Keep this in sync with .cpu-usage-view Ditto. > Source/WebInspectorUI/UserInterface/Views/StackedColumnChart.js:48 > +// StackedColumnChart creates a chart with filled columns each stratified with sections. > +// > +// Initialize the chart with a size. > +// To populate with data, first initialize the sections. The class names you > +// provide for the segments will allow you to style them. You can then include > +// a new set of (x, totalHeight, w, [h1, h2, h3]) points in the chart via `addColumnSet`. > +// The order of `h` values must be in the same order as the sections. > +// The `y` value to be used for each column is `totalHeight - h`. > +// > +// SVG: > +// > +// - There is a single rect for each bar and each section. > +// - Each bar extends all the way down to the bottom, they are layered such > +// that the rects for early sections overlap the sections for later sections. > +// > +// <div class="stacked-column-chart"> > +// <svg viewBox="0 0 800 75"> > +// <rect class="section-class-name-3" width="<w>" height="<h3>" transform="translate(<x>, <y>)" /> > +// <rect class="section-class-name-2" width="<w>" height="<h2>" transform="translate(<x>, <y>)" /> > +// <rect class="section-class-name-1" width="<w>" height="<h1>" transform="translate(<x>, <y>)" /> > +// ... > +// </svg> > +// </div> I like this! We should write high-level descriptions like these more often.
Created attachment 362542 [details] [Image] RTL - flipped text
Created attachment 362544 [details] [Image] Yellow bar has green border I expect yellow bars to have dark yellow (brown) borders, not green. For the stacked graphs, the top border of the yellow bar should be dark yellow.
I'm done for now. I can do another pass when I see my comments addressed or explained why they weren't.
Comment on attachment 362328 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362328&action=review >> Source/WebInspectorUI/ChangeLog:63 >> + (.cpu-usage-view .line-chart > svg > path): Deleted. > > Nit: line charts usually have just lines without any background. What you're using looks like an area chart to me. > https://duckduckgo.com/?q=line+chart&t=h_&iar=images&iax=images&ia=images > https://duckduckgo.com/?q=area+chart&t=h_&iar=images&iax=images&ia=images Yep, I'll follow this up with something that renames LineChart to AreaChart appropriately. This has come up in the past. >> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:46 >> + this._workers = null; > > We could use an empty array here. I want to reduce the memory footprint of each CPUTimelineRecord by not creating an array if it is not needed, since workers are very rare. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:103 >> +} > > Where's the divider in the UI? Currently we don't have one, but this will go between this an the next chart. I can remove this for now. I'd been using it with prototypes of a second chart. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:205 >> + bottom: 0; > > Nit: I'd put `pointer-events: none` here instead of each marker. Done! >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:164 >> + this._breakdownLegendElement.classList.add("legend"); > > Main Thread section with the pie chart and a legend is nearly identical to the ones in the Memory tool. Have you considered abstracting them out in a separate view? My opinion has always been to avoid abstraction until we've done something 3 or more times. So if we do something similar a 3rd time that would be enough of a trend that it would be worth abstracting. We did something similiar with the ResourceDetailViews in the Network Table. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219 >> + const threadCPUUsageViewHeight = 65; // Keep this in sync with .cpu-usage-view > > Have you considered doing something like this? > > element.style.setProperty("--cpu-usage-height", cpuUsageViewHeight + "px") This is a great idea! >> Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:61 >> + transform: scaleX(-1); > > This flips text labels as well, e.g. "50%". Good catch. I had done RTL but missed this. I'll look into this.
Comment on attachment 362328 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362328&action=review >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219 >>> + const threadCPUUsageViewHeight = 65; // Keep this in sync with .cpu-usage-view >> >> Have you considered doing something like this? >> >> element.style.setProperty("--cpu-usage-height", cpuUsageViewHeight + "px") > > This is a great idea! It really is. A quick search for "Keep this in sync" shows a handful of cases in our CSS files. I'm sure there are other magic numbers from JavaScript lurking in Web Inspector CSS as well. I filed <https://webkit.org/b/194883> to track this.
Created attachment 362655 [details] [PATCH] Proposed Fix Addressed earlier comments and rebaslined.
(In reply to Nikita Vasilyev from comment #14) > Created attachment 362544 [details] > [Image] Yellow bar has green border > > I expect yellow bars to have dark yellow (brown) borders, not green. > For the stacked graphs, the top border of the yellow bar should be dark > yellow. I still want this to be addressed at some point. (In reply to Joseph Pecoraro from comment #16) > Comment on attachment 362328 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362328&action=review > > >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:164 > >> + this._breakdownLegendElement.classList.add("legend"); > > > > Main Thread section with the pie chart and a legend is nearly identical to the ones in the Memory tool. Have you considered abstracting them out in a separate view? > > My opinion has always been to avoid abstraction until we've done something 3 > or more times. So if we do something similar a 3rd time that would be enough > of a trend that it would be worth abstracting. We did something similiar > with the ResourceDetailViews in the Network Table. Sounds good to me!
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review A few minor code style comments. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:429 > + let graphMax = (layoutMax * 1.05); Nit: unnecessary parentheses. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:436 > + return graphHeight - (((value - graphMin) / graphMax) * graphHeight); Nit: graphHeight - ((value - graphMin) / graphMax * graphHeight); > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:457 > + let minimumDisplayablePercentByTwo = Math.ceil((minimumMarkerTextHeight * percentPerPixel) / 2) * 2; Nit: minimumMarkerTextHeight * percentPerPixel / 2 Do you find extra parentheses make it more readable? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:460 > + let minimumDisplayablePercentByFive = Math.ceil((minimumMarkerTextHeight * percentPerPixel) / 5) * 5; Ditto. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:574 > + markRecordEntries(scriptRecords, (record) => { > + return WI.CPUTimelineView.SampleType.Script; > + }); Nit: a regular (non-arrow) function could be used instead since you don't have any `this` inside. Have we agreed on something like this before? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:576 > + markRecordEntries(layoutRecords, (record) => { Ditto.
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review Thanks for the additional comments! >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:429 >> + let graphMax = (layoutMax * 1.05); > > Nit: unnecessary parentheses. Will update. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:436 >> + return graphHeight - (((value - graphMin) / graphMax) * graphHeight); > > Nit: graphHeight - ((value - graphMin) / graphMax * graphHeight); Good point, I'll use graphHeight here. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:457 >> + let minimumDisplayablePercentByTwo = Math.ceil((minimumMarkerTextHeight * percentPerPixel) / 2) * 2; > > Nit: minimumMarkerTextHeight * percentPerPixel / 2 > > Do you find extra parentheses make it more readable? Yeah, while the expressions are the same I find the clarity of what is being divided in half to be useful. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:574 >> + }); > > Nit: a regular (non-arrow) function could be used instead since you don't have any `this` inside. Have we agreed on something like this before? I don't think we have any preferred style in this case. I tend to use arrow functions inline because they are just a little shorter. I do not believe using an arrow function instead of a plain function hurts performance (time or memory) in these cases. Maybe negligibly for parse times.
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:436 >>> + return graphHeight - (((value - graphMin) / graphMax) * graphHeight); >> >> Nit: graphHeight - ((value - graphMin) / graphMax * graphHeight); > > Good point, I'll use graphHeight here. Err, graphMin is zero so I optimized it out. graphMin was non-zero for Memory to "zoom in" on the changes.
Created attachment 362801 [details] [PATCH] Proposed Fix Updated.
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review > Source/WebInspectorUI/ChangeLog:39 > + (.timeline-view.cpu > .content > .overview > .divider): > + (body[dir=ltr] .timeline-view.cpu > .content > .overview > .divider): > + (body[dir=rtl] .timeline-view.cpu > .content > .overview > .divider): NIT: these don't exist in this patch anymore > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:59 > + this._workers.push({targetId: thread.targetId, usage: thread.usage}); Is there a reason not to just `this._workers.push(thread)`? It doesn't look like the `threads` data is stored anywhere else, so there shouldn't be a risk of manipulation. > Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:81 > + get workers() { return this._workers; } I think something like `workersData` may be slightly clearer, especially since the value is not a typical model object. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:34 > + margin-bottom: 10px; NIT: I normally put `margin` before `padding.` > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:74 > + z-index: calc(var(--timeline-marker-z-index) + 1); NIT: I normally put `z-index` alongside positional properties (e.g. `top`). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:119 > + background: var(--cpu-idle-fill-color); NIT: `background-color`? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:191 > + top: 0; > + left: 0; > + right: 0; > + bottom: 0; My typical ordering is `top`, `right`, `bottom`, `left` to match the ordering of most shorthands. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:204 > + text-align: right; `text-align: end`? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:209 > + text-align: left; Ditto (>204). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:54 > + } NIT: we should include some sort of `console.error("Invalid sample type", type);` and a fallback value. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:58 > + static get cpuUsageViewHeight() { return 150; } > + static get threadCPUUsageViewHeight() { return 65; } Are these considered "simple" getters? Should they be on their own line? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:120 > + this.element.style.setProperty("--cpu-usage-stacked-view-height", CPUTimelineView.cpuUsageViewHeight + "px"); > + this.element.style.setProperty("--cpu-usage-view-height", CPUTimelineView.threadCPUUsageViewHeight + "px"); NIT: `WI` prefix? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:128 > + function createChartContainer(parentElement, subtitle, tooltip) { Would `text` and `title` be more accurate rather than `subtitle` (what's the title in this case?) and `tooltip`? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:154 > + labelElement.textContent = label; Rather than pass in `label` yourself, you could just call `WI.CPUTimelineView.displayNameForSampleType` on `swatchClass` (which you could rename to `sampleType` for consistency). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:162 > + let breakdownTooltip = WI.UIString("Breakdown of time spent on the main thread"); Style: `const`. You could also inline this. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:165 > + this._breakdownChart.segments = Object.values(WI.CPUTimelineView.SampleType); Nice! > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:175 > + this._breakdownLegendScriptElement = appendLegendRow.call(this, this._breakdownLegendElement, WI.CPUTimelineView.SampleType.Script, WI.CPUTimelineView.displayNameForSampleType(WI.CPUTimelineView.SampleType.Script)); > + this._breakdownLegendLayoutElement = appendLegendRow.call(this, this._breakdownLegendElement, WI.CPUTimelineView.SampleType.Layout, WI.CPUTimelineView.displayNameForSampleType(WI.CPUTimelineView.SampleType.Layout)); > + this._breakdownLegendPaintElement = appendLegendRow.call(this, this._breakdownLegendElement, WI.CPUTimelineView.SampleType.Paint, WI.CPUTimelineView.displayNameForSampleType(WI.CPUTimelineView.SampleType.Paint)); > + this._breakdownLegendStyleElement = appendLegendRow.call(this, this._breakdownLegendElement, WI.CPUTimelineView.SampleType.Style, WI.CPUTimelineView.displayNameForSampleType(WI.CPUTimelineView.SampleType.Style)); These don't need to use `.call(this` as `this` isn't used inside `appendLegendRow`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:187 > + this._timelineRuler.updateLayout(WI.View.LayoutReason.Resize); Why is this needed? It should call `layout` immediately after it's own `initialLayout`. If you need some value calculate by the ruler inside it's own `layout` or `sizeDidChange`, you should put that logic in a `didLayoutSubtree` on this object instead. If the ruler is wrong _unless_ this is called, then that's a bug on the ruler that we should fix. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:230 > + console.assert(secondsPerPixel, "Ruler should always have a valid seconds per pixel."); I think this check is unnecessary. If anything, it should be inside `WI.TimelineRuler`, not here (we should never expect a ruler to be in this sort of state). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:237 > + let originalDiscontinuities = discontinuities.slice(0); NIT: the `0` is unnecessary. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:249 > + if (!nonIdleSamplesCount) { Style: since we already have an `else`, you should switch the order of these (the "meaty" logic is inside the current `else` anyways, so bringing it closer to the data source may be easier to read). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:262 > + this._breakdownLegendScriptElement.textContent = `${Number.percentageString(percentScript)} (${samplingData.samplesScript})`; > + this._breakdownLegendLayoutElement.textContent = `${Number.percentageString(percentLayout)} (${samplingData.samplesLayout})`; > + this._breakdownLegendPaintElement.textContent = `${Number.percentageString(percentPaint)} (${samplingData.samplesPaint})`; > + this._breakdownLegendStyleElement.textContent = `${Number.percentageString(percentStyle)} (${samplingData.samplesStyle})`; Are parenthesis localizable? I've had issues with "%" in the past, so we should probably use a `UIString` here just in case. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:280 > + let workers = new Map; This name makes me think that it's an array. Perhaps `workerDataMap`? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:299 > + let {usage, mainThreadUsage, workerThreadUsage, webkitThreadUsage, unknownThreadUsage} = record; Style: I don't like destructuring with a non-POD object. It makes it harder to track down usage of getters/setters (e.g. can't search for ".mainThreadUsage"). I'd prefer it if you called each from the `record` (e.g. `record.mainThreadUsage`) instead. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:309 > + dataPoints.push({time: startDiscontinuity.startTime, mainThreadUsage: previousDataPoint.mainThreadUsage, workerThreadUsage: previousDataPoint.workerThreadUsage, webkitThreadUsage: previousDataPoint.webkitThreadUsage, unknownThreadUsage: previousDataPoint.unknownThreadUsage, usage: previousDataPoint.usage}); Style: please separate this onto separate lines for readability. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:314 > + dataPoints.push({time: startDiscontinuity.startTime, mainThreadUsage: 0, workerThreadUsage: 0, webkitThreadUsage: 0, unknownThreadUsage: 0, usage: 0}); > + dataPoints.push({time: endDiscontinuity.endTime, mainThreadUsage: 0, workerThreadUsage: 0, webkitThreadUsage: 0, unknownThreadUsage: 0, usage: 0}); > + dataPoints.push({time: endDiscontinuity.endTime, mainThreadUsage, workerThreadUsage, webkitThreadUsage, unknownThreadUsage, usage}); Ditto (>309). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:338 > + workerData = {discontinuities: originalDiscontinuities.slice(0), recordsCount: 0, dataPoints: [], min: Infinity, max: -Infinity, average: 0}; Ditto (>237). Ditto (>309). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:340 > + while (workerData.discontinuities.length && workerData.discontinuities[0].endTime <= graphStartTime) > + workerData.discontinuities.shift(); Rather than continuously `shift`ing, perhaps we can keep an index and do a giant `splice` at the end? I doubt this array will ever have more than a few items in a row anyways, so maybe not worth it. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:350 > + while (workerData.discontinuities.length && workerData.discontinuities[0].endTime < time) > + endDiscontinuity = workerData.discontinuities.shift(); Ditto (>339). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:372 > + mainThreadAverage /= visibleRecords.length; > + webkitThreadAverage /= visibleRecords.length; > + unknownThreadAverage /= visibleRecords.length; Shouldn't these be dividing by the number of records for that category? The min/max of these categories is based on data specific to that category. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:374 > + for (let [workerId, workerData] of workers) NIT: you can use `workers.values()` and avoid the array destructure. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:405 > + let even = (hundreds % 2) === 0; You could inline this, as well as reversing the `if` so you can drop the `=== 0`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:409 > + return [bottom, top]; I think we should always be showing a 100% marker no matter what the value is (just to give a sense of "perspective"). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:418 > + function layoutView(view, property, graphHeight, {dataPoints, layoutMax, min, max, average}) { Since `max` and `min` are already defined above, we should use a different name or use an object accessor (e.g. not destructure) to avoid shadowing "issues". `layoutMax` is the same for all calls of `layoutView`, so could we define it once before this function and capture it (or just use the underlying `max` with a non-shadowed value)? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:426 > + let isAllThreadsGraph = property === null; Style: `!property`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:428 > + let graphMin = 0; Can we remove this? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:-176 > - let size = new WI.Size(xScale(graphEndTime), cpuUsageViewHeight); Having this here ensures that the height used for updating the chart is the same as the height used by the scale. I'd prefer if you kept this as is (albeit with changing `cpuUsageViewHeight` to `height`). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:454 > + const minimumMarkerTextHeight = 17; Should this be kept in sync with some CSS? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:473 > + labelElement.innerText = Number.percentageString(value / 100, 0); NIT: this could use a `const precision = 0;`. I had to look up the function to see what it was for :( > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:484 > + layoutView(this._cpuUsageView, null, CPUTimelineView.cpuUsageViewHeight, {dataPoints, layoutMax, min, max, average}); > + layoutView(this._mainThreadUsageView, "mainThreadUsage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints, layoutMax, min: mainThreadMin, max: mainThreadMax, average: mainThreadAverage}); > + layoutView(this._webkitThreadUsageView, "webkitThreadUsage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints, layoutMax, min: webkitThreadMin, max: webkitThreadMax, average: webkitThreadAverage}); > + layoutView(this._unknownThreadUsageView, "unknownThreadUsage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints, layoutMax, min: unknownThreadMin, max: unknownThreadMax, average: unknownThreadAverage}); Ditto (>119). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:497 > + layoutView(workerView, "usage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints: workerData.dataPoints, layoutMax, min: workerData.min, max: workerData.max, average: workerData.average}); Ditto (>119). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:517 > + case WI.ScriptTimelineRecord.EventType.ScriptEvaluated: > + case WI.ScriptTimelineRecord.EventType.APIScriptEvaluated: > + case WI.ScriptTimelineRecord.EventType.ObserverCallback: > + case WI.ScriptTimelineRecord.EventType.EventDispatched: > + case WI.ScriptTimelineRecord.EventType.MicrotaskDispatched: > + case WI.ScriptTimelineRecord.EventType.TimerFired: > + case WI.ScriptTimelineRecord.EventType.AnimationFrameFired: A comment explaining that "these types of records are the only ones that do any work" would be nice :) You could also remove the other `case`s and just `return false;` since we don't "care" about them anyways (unless you want to make sure new record types get "considered", in which case there should be an assertion/error). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:539 > + case WI.LayoutTimelineRecord.EventType.RecalculateStyles: > + case WI.LayoutTimelineRecord.EventType.ForcedLayout: > + case WI.LayoutTimelineRecord.EventType.Layout: > + case WI.LayoutTimelineRecord.EventType.Paint: > + case WI.LayoutTimelineRecord.EventType.Composite: Ditto (>511). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:549 > + let millisecondDuration = (millisecondEndTime - millisecondStartTime); Style: unnecessary parenthesis. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:568 > + samples[t - millisecondStartTime] = value; Should we assert that no two records have the same `sample` index? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:593 > + let samplesIdle = 0; > + let samplesScript = 0; > + let samplesLayout = 0; > + let samplesPaint = 0; > + let samplesStyle = 0; Rather than prefixing each of these with `samples`, how about flipping the name and adding a `Count` suffix (e.g. `idleCount`)? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:594 > + for (let i = 0; i < samples.length; ++i) { Style: `for (let sample of samples)`? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:596 > + case undefined: Clever! > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:615 > + samples, The only time this is used is actually just for it's `length`, so how about returning it as `sampleCount: samples.ength` instead? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:659 > + Script: "sample-type-script", > + Layout: "sample-type-layout", > + Paint: "sample-type-paint", > + Style: "sample-type-style", Is this in a particular order, or can it be alphabetized? If the former, please add a comment explaining that. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:42 > + -webkit-padding-start: 15px; NIT: I normally put this alongside other `padding` properties. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:48 > + border-right: var(--cpu-usage-view-details-border-end); NIT: you can also use `-webkit-border-end`. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.js:32 > + this.element.classList.add("cpu-usage-stacked-view"); NIT: it seems like all of the logic in this constructor can be moved to an `initialLayout`. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.js:47 > + this._detailsElement.appendChild(document.createElement("br")); > + this._detailsAverageElement = this._detailsElement.appendChild(document.createElement("span")); > + this._detailsElement.appendChild(document.createElement("br")); > + this._detailsMaxElement = this._detailsElement.appendChild(document.createElement("span")); > + this._detailsElement.appendChild(document.createElement("br")); > + this._detailsMinElement = this._detailsElement.appendChild(document.createElement("span")); > + this._updateDetails(NaN, NaN); I'd add newlines in between each of these, as it's quite hard to read as is. > Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.js:54 > + this._chart.initializeSections(["main-thread-usage", "worker-thread-usage", "total-usage"]); I'd put this before the `addSubview` call. > Source/WebInspectorUI/UserInterface/Views/LegacyCPUTimelineView.js:104 > + this.element.style.setProperty("--cpu-usage-view-height", LegacyCPUTimelineView.cpuUsageViewHeight + "px"); NIT: `WI` prefix? > Source/WebInspectorUI/UserInterface/Views/LegacyCPUTimelineView.js:178 > + let size = new WI.Size(xScale(graphEndTime), LegacyCPUTimelineView.cpuUsageViewHeight); Ditto (>104). > Source/WebInspectorUI/UserInterface/Views/View.js:134 > + removeUnparentedSubview(view) Personally, I'd rather relax the assertion inside `removeSubview`, especially considering how prevalent this pattern is. I think the assertion should just check that the parent view's element contains the child view's element (not necessarily immediate parent/child).
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review >> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:81 >> + get workers() { return this._workers; } > > I think something like `workersData` may be slightly clearer, especially since the value is not a typical model object. Good idea. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:191 >> + bottom: 0; > > My typical ordering is `top`, `right`, `bottom`, `left` to match the ordering of most shorthands. There are 35 instances of the pyramid, and only 6 of top/right/bottom/left. I think that is our norm. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:204 >> + text-align: right; > > `text-align: end`? Cool! >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:58 >> + static get threadCPUUsageViewHeight() { return 65; } > > Are these considered "simple" getters? Should they be on their own line? I think until we get static properties this is good enough. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:120 >> + this.element.style.setProperty("--cpu-usage-view-height", CPUTimelineView.threadCPUUsageViewHeight + "px"); > > NIT: `WI` prefix? I've been using the non-WI prefixed within the class like this. I think its a good idea to try using this part of the language inside the class. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:128 >> + function createChartContainer(parentElement, subtitle, tooltip) { > > Would `text` and `title` be more accurate rather than `subtitle` (what's the title in this case?) and `tooltip`? Hmm, perhaps, this is shared with Memory timeline lets update after. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:154 >> + labelElement.textContent = label; > > Rather than pass in `label` yourself, you could just call `WI.CPUTimelineView.displayNameForSampleType` on `swatchClass` (which you could rename to `sampleType` for consistency). Yeah this was because `appendLegendRow` could have been shared across multiple charts. We aren't going to do this in the CPU timeline though so I'll reduce it. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:175 >> + this._breakdownLegendStyleElement = appendLegendRow.call(this, this._breakdownLegendElement, WI.CPUTimelineView.SampleType.Style, WI.CPUTimelineView.displayNameForSampleType(WI.CPUTimelineView.SampleType.Style)); > > These don't need to use `.call(this` as `this` isn't used inside `appendLegendRow`. Nice >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:187 >> + this._timelineRuler.updateLayout(WI.View.LayoutReason.Resize); > > Why is this needed? It should call `layout` immediately after it's own `initialLayout`. If you need some value calculate by the ruler inside it's own `layout` or `sizeDidChange`, you should put that logic in a `didLayoutSubtree` on this object instead. If the ruler is wrong _unless_ this is called, then that's a bug on the ruler that we should fix. This came out of moving this code to initialLayout. This fixes the case where: 1. Inspect some page 2. Show Timeline Tab (on Scripts timeline) 3. Make a recording 4. Switch to CPU Timeline with values => First draw is incorrect I think this is because CPUTimelineView.prototype.layout will happen after this initialLayout, which is before the initialLayout of the TimelineRuler. Meaning we won't have expected TimelineRuler values. This forces the TimelineRuler to layout now, in our initialLayout, before our layout. I've added a comment. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:237 >> + let originalDiscontinuities = discontinuities.slice(0); > > NIT: the `0` is unnecessary. Nice! >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:280 >> + let workers = new Map; > > This name makes me think that it's an array. Perhaps `workerDataMap`? Sounds good. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:299 >> + let {usage, mainThreadUsage, workerThreadUsage, webkitThreadUsage, unknownThreadUsage} = record; > > Style: I don't like destructuring with a non-POD object. It makes it harder to track down usage of getters/setters (e.g. can't search for ".mainThreadUsage"). I'd prefer it if you called each from the `record` (e.g. `record.mainThreadUsage`) instead. Hmm, I see your point but we do this all over and a unique name like mainThreadUsage is easy to search for. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:314 >> + dataPoints.push({time: endDiscontinuity.endTime, mainThreadUsage, workerThreadUsage, webkitThreadUsage, unknownThreadUsage, usage}); > > Ditto (>309). This dramatically increases the number of lines and makes this a bit harder to read. I did it for the one above but not the others. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:340 >> + workerData.discontinuities.shift(); > > Rather than continuously `shift`ing, perhaps we can keep an index and do a giant `splice` at the end? I doubt this array will ever have more than a few items in a row anyways, so maybe not worth it. Yeah, we do the same thing with discontinuities everywhere. The shift is indeed wasteful but there are so few. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:372 >> + unknownThreadAverage /= visibleRecords.length; > > Shouldn't these be dividing by the number of records for that category? The min/max of these categories is based on data specific to that category. This is the average of each thread over the entire recording, so all CPU records need to be taken into account. If a worker thread popped up in 1 out of 10 samples with 100% CPU, it should show as an average of 10% CPU not 100% CPU for the selected range. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:409 >> + return [bottom, top]; > > I think we should always be showing a 100% marker no matter what the value is (just to give a sense of "perspective"). This sounds like something we can improve in a follow-up. I think I agree, 100% is such a human value to want to have it may be worth showing. Likewise the normal website ends up well below 200. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:-176 >> - let size = new WI.Size(xScale(graphEndTime), cpuUsageViewHeight); > > Having this here ensures that the height used for updating the chart is the same as the height used by the scale. I'd prefer if you kept this as is (albeit with changing `cpuUsageViewHeight` to `height`). I don't understand the advantage, but I'll change this back. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:568 >> + samples[t - millisecondStartTime] = value; > > Should we assert that no two records have the same `sample` index? It is expected that this happen. A script that forces a layout will first write the Script samples, and then override with more specific Layout samples. For example: Initial: [ ------, ------, ------, ------, ------ ] Script Samples: [ ------, Script, Script, Script, ------ ] Layout Samples: [ ------, Script, Layout, Script, ------ ] This is why we do scripts first and override with Layout after. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:615 >> + samples, > > The only time this is used is actually just for it's `length`, so how about returning it as `sampleCount: samples.ength` instead? It will be used in later patches. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:659 >> + Style: "sample-type-style", > > Is this in a particular order, or can it be alphabetized? If the former, please add a comment explaining that. The UI (and code) always follows this order. >> Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.css:48 >> + border-right: var(--cpu-usage-view-details-border-end); > > NIT: you can also use `-webkit-border-end`. Nice! >> Source/WebInspectorUI/UserInterface/Views/View.js:134 >> + removeUnparentedSubview(view) > > Personally, I'd rather relax the assertion inside `removeSubview`, especially considering how prevalent this pattern is. I think the assertion should just check that the parent view's element contains the child view's element (not necessarily immediate parent/child). Good idea.
Created attachment 362958 [details] [PATCH] Proposed Fix
Comment on attachment 362655 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362655&action=review >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:187 >>> + this._timelineRuler.updateLayout(WI.View.LayoutReason.Resize); >> >> Why is this needed? It should call `layout` immediately after it's own `initialLayout`. If you need some value calculate by the ruler inside it's own `layout` or `sizeDidChange`, you should put that logic in a `didLayoutSubtree` on this object instead. If the ruler is wrong _unless_ this is called, then that's a bug on the ruler that we should fix. > > This came out of moving this code to initialLayout. > > This fixes the case where: > 1. Inspect some page > 2. Show Timeline Tab (on Scripts timeline) > 3. Make a recording > 4. Switch to CPU Timeline with values > => First draw is incorrect > > I think this is because CPUTimelineView.prototype.layout will happen after this initialLayout, which is before the initialLayout of the TimelineRuler. Meaning we won't have expected TimelineRuler values. This forces the TimelineRuler to layout now, in our initialLayout, before our layout. > > I've added a comment. In that case, how about we use `didLayoutSubtree`? That was added to "fix" this exact scenario. >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:372 >>> + unknownThreadAverage /= visibleRecords.length; >> >> Shouldn't these be dividing by the number of records for that category? The min/max of these categories is based on data specific to that category. > > This is the average of each thread over the entire recording, so all CPU records need to be taken into account. > > If a worker thread popped up in 1 out of 10 samples with 100% CPU, it should show as an average of 10% CPU not 100% CPU for the selected range. Ah, I see. So it's more of a total/overall average, rather than a categorical average? >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:568 >>> + samples[t - millisecondStartTime] = value; >> >> Should we assert that no two records have the same `sample` index? > > It is expected that this happen. > > A script that forces a layout will first write the Script samples, and then override with more specific Layout samples. For example: > > Initial: [ ------, ------, ------, ------, ------ ] > Script Samples: [ ------, Script, Script, Script, ------ ] > Layout Samples: [ ------, Script, Layout, Script, ------ ] > > This is why we do scripts first and override with Layout after. Gotcha. Makes sense. Maybe we should assert that what we override is a `WI.ScriptTimelineRecord` type? I just want to make sure that any future readers of this code can somewhat understand the reasoning without having to know "everything" about timeline records (your response will definitely help).
> In that case, how about we use `didLayoutSubtree`? That was added to "fix" > this exact scenario. This would mean putting the entire `layout` contents into `didLayoutSubtree`. I don't think that is what we want, unless I'm misunderstanding what you're saying. > > >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:372 > >>> + unknownThreadAverage /= visibleRecords.length; > >> > >> Shouldn't these be dividing by the number of records for that category? The min/max of these categories is based on data specific to that category. > > > > This is the average of each thread over the entire recording, so all CPU records need to be taken into account. > > > > If a worker thread popped up in 1 out of 10 samples with 100% CPU, it should show as an average of 10% CPU not 100% CPU for the selected range. > > Ah, I see. So it's more of a total/overall average, rather than a > categorical average? Its the average CPU usage for that thread/group over the selection range. That means it must take all samples for that thread group into account. For Workers we have a separate number of samples per-worker, because those come and go and have a different lifetime. For these global groups, they are global and area treated as always around. > >>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:568 > >>> + samples[t - millisecondStartTime] = value; > >> > >> Should we assert that no two records have the same `sample` index? > > > > It is expected that this happen. > > > > A script that forces a layout will first write the Script samples, and then override with more specific Layout samples. For example: > > > > Initial: [ ------, ------, ------, ------, ------ ] > > Script Samples: [ ------, Script, Script, Script, ------ ] > > Layout Samples: [ ------, Script, Layout, Script, ------ ] > > > > This is why we do scripts first and override with Layout after. > > Gotcha. Makes sense. Maybe we should assert that what we override is a > `WI.ScriptTimelineRecord` type? I just want to make sure that any future > readers of this code can somewhat understand the reasoning without having to > know "everything" about timeline records (your response will definitely > help). I can add a comment about what this code is trying to do.
Comment on attachment 362958 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362958&action=review r=me, nice work :) > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:143 > + function appendLegendRow(legendElement, sampleType, label) { NIT: `label` isn't used. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:159 > + Style: unnecessary newline. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:190 > + // Cause the TimelineRuler to layout now so we will have some of its > + // important properties initialized for our layout. > + this._timelineRuler.updateLayout(WI.View.LayoutReason.Resize); This is an interesting case, because `didLayoutSubtree` would be a solution to this, but because we modify some of the subviews, we'd effectively cause two layouts for each one. I suppose it's better to have a single view do two layouts initially rather than have most (if not all) subviews do two layouts per layout. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219 > + this._workerViews = []; Should this be defined (maybe just as `null`) in the constructor? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:234 > + if (!secondsPerPixel) > + return; Given that you force a layout above, is it ever possible to have this happen? My comment in the previous patch was more about moving the assertion, not removing it entirely. I do agree that this shouldn't be reachable. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:253 > + if (!nonIdleSamplesCount) { Style: since we already have an `else`, you should switch the order of these (the "meaty" logic is inside the current `else` anyways, so bringing it closer to the data source may be easier to read). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:266 > + this._breakdownLegendStyleElement.textContent = `${Number.percentageString(percentStyle)} (${samplingData.samplesStyle})`; Are parenthesis localizable? I've had issues with "%" in the past, so we should probably use a `UIString` here just in case. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:393 > + for (let [workerId, workerData] of workersDataMap) NIT: you can use `workersDataMap.values()` and avoid the array destructure. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:424 > + let even = (hundreds % 2) === 0; NIT: you could inline this, as well as reversing the `if` so you can drop the `=== 0`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:441 > + function layoutView(view, property, graphHeight, {dataPoints, min, max, average}) { Since `max` and `min` are already defined above, we should use a different name or use an object accessor (e.g. not destructure) to avoid shadowing "issues". > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:449 > + let isAllThreadsGraph = property === null; Style: `!property`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:476 > + const minimumMarkerTextHeight = 17; Should this be kept in sync with some CSS? How was this number achieved? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:495 > + labelElement.classList.add("label"); > + const precision = 0; Style: missing newline. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:516 > + layoutView(workerView, "usage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints: workerData.dataPoints, min: workerData.min, max: workerData.max, average: workerData.average}); Can we just pass `workerData` rather than try to remove just a few pieces of data, especially since `layoutView` is also defined in this same file/function, or are you worried that the keys/names might change and break things? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:621 > + let samplesIdle = 0; Rather than prefixing each of these with `samples`, how about flipping the name and adding a `Count` suffix (e.g. `idleCount`)? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:626 > + for (let i = 0; i < samples.length; ++i) { Style: `for (let sample of samples)` > Source/WebInspectorUI/UserInterface/Views/View.js:134 > + removeUnparentedSubview(view) This should be removed.
Comment on attachment 362958 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=362958&action=review >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219 >> + this._workerViews = []; > > Should this be defined (maybe just as `null`) in the constructor? 1 per TimelineView, I'll keep an empty array for simplicity. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:234 >> + return; > > Given that you force a layout above, is it ever possible to have this happen? My comment in the previous patch was more about moving the assertion, not removing it entirely. I do agree that this shouldn't be reachable. I was seeing assertions occasionally, but never any drawing issues. This seems safe enough, to prevent any drawing using NaN / Infinity values. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:393 >> + for (let [workerId, workerData] of workersDataMap) > > NIT: you can use `workersDataMap.values()` and avoid the array destructure. Repeated comments like so many of these are not particularly helpful. Nit / style comments can be ignored unless you feel strongly. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:476 >> + const minimumMarkerTextHeight = 17; > > Should this be kept in sync with some CSS? How was this number achieved? This number was achieved by measuring at what point would the text of a marker would feel cramped / clipped / annoying. Hence the variable name. It could be kept up with the text size but that feels overkill. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:516 >> + layoutView(workerView, "usage", CPUTimelineView.threadCPUUsageViewHeight, {dataPoints: workerData.dataPoints, min: workerData.min, max: workerData.max, average: workerData.average}); > > Can we just pass `workerData` rather than try to remove just a few pieces of data, especially since `layoutView` is also defined in this same file/function, or are you worried that the keys/names might change and break things? I was worried about keys not matching. I'd like to keep the style the same as the calls above. >> Source/WebInspectorUI/UserInterface/Views/View.js:134 >> + removeUnparentedSubview(view) > > This should be removed. Oops. I thought I did!
https://trac.webkit.org/r242073
<rdar://problem/48391329>