Created attachment 255859 [details] [Image] Subpixel problems * SUMMARY Rendering Frame bars appear misaligned and contain gaps when displaying small task segments. The Rendering Frames overview graph has a fixed height of 108px. Frame time is shown on the vertical axis with a minimum/maximum height of 0.0185 s and 0.037 s. Thus each pixel on the y-axis has a value between 0.171 ms - 0.342 ms. The duration of a single task can frequently fall below this resolution, resulting in subpixel segment heights. After rasterization and applying CSS styles (our 1px segment separator), these small tasks are invisible, appearing as gaps in the frame. It is especially noticeable when the gap occurs at the bottom of the frame, making it appear to float above the horizontal axis (see screenshot). * NOTE Some tasks will inevitably be too small to display. The ideal algorithm would: 1. Throw out tasks that are too small. 2. Pad tasks that are very close to the minimum resolution in order to display them (borrowing "rounded" time from larger tasks). 3. Show all frames, regardless of how small. A brief frame that would be less than a pixel high should be padded to a minimum frame height. 4. Keep the overall frame height and ratio of displayed tasks as accurate as possible.
<rdar://problem/21620063>
Created attachment 255980 [details] [Image] Results after applying new algorithm Three timeline recordings rendered with the new frame-segment algorithm. Shown alongside the same data rendered with the current algorithm for comparison.
Created attachment 256217 [details] [Patch] Proposed Fix
Comment on attachment 256217 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review Sounds good! r=me > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:99 > + var currentSegment; Style: lets do an = null. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:116 > + if (currentSegment.duration < secondsPerBlock) > + invisibleSegments.push({segment: segments.lastValue, index: segments.length - 1}); Nit: segments.lastValue at this point could just be "currentSegment". > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:156 > + // After grouping sub-block tasks, a second is needed to handle those groups that are still beneath the minimum Grammaro: "a second is needed" => "a second pass is needed" > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:186 > + }, this); Unnecessary "this". > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:197 > + }, this); Unnecessary "this". > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:227 > + this._record.__displayData.segments.forEach(function(segment) { > + var element = document.createElement("div"); > + this._updateElementPosition(element, segment.duration / this._record.__displayData.frameDuration, "height"); > + element.classList.add("duration", segment.taskType); > + frameElement.insertBefore(element, frameElement.firstChild); > }, this); I wonder if it would be better to append in reverse order to avoid a whole bunch of insertBefores? I'm not sure if there is a possible performance cost, especially for very small lists (1-3).
Comment on attachment 256217 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:804 > + return Math.round(num / step) * step; weird spacing. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:170 > + var available = previous && next ? previous.remainder + next.remainder : (previous ? previous.remainder : next.remainder); Ow, my brain. Try to be consistent about naming (i.e., availableDuration, previousSegment, nextSegment) > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:173 > + var targetSegment = previous && next ? (previous.duration > next.duration ? previous : next) : previous || next; Ow, my brain. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:179 > + var neighbors = previous && next ? (previous.remainder > next.remainder ? [previous, next] : [next, previous]) : [previous || next] I've written this type of line before. You will regret it next year. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:199 > + return {frameDuration, segments}; Cool!
Comment on attachment 256217 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256217&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:179 >> + var neighbors = previous && next ? (previous.remainder > next.remainder ? [previous, next] : [next, previous]) : [previous || next] > > I've written this type of line before. You will regret it next year. I agree, nested ternaries are pretty nasty. I'll rewrite them as if-else for clarity.
Created attachment 256235 [details] [Patch] Proposed Fix
Comment on attachment 256235 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256235&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:104 > + } > + var roundedDuration = Math.roundTo(segment.duration, secondsPerBlock); Nit: Newline between these lines. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:123 > + Object.keys(WebInspector.RenderingFrameTimelineRecord.TaskType).forEach(function(key) { for (var key in WebInspector.RenderingFrameTimelineRecord.TaskType) { > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:140 > + } > + if (currentSegment.duration >= secondsPerBlock) Nit: Newline between these lines. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:159 > + invisibleSegments.forEach(function(item) { for (var item of invisibleSegments) { > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:238 > + this._record.__displayData.segments.forEach(function(segment) { for (var segment of this._record.__displayData.segments)
Created attachment 256253 [details] [Patch] Proposed Fix
Comment on attachment 256253 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192 > + adjacentSegments.forEach(function(adjacentSegment) { Not: Could have also been converted to for (..of..).
Comment on attachment 256253 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192 >> + adjacentSegments.forEach(function(adjacentSegment) { > > Not: Could have also been converted to for (..of..). What are our style guidelines with regard to Array.prototype.forEach? I see it being used with and without the optional thisArg.
Comment on attachment 256253 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=256253&action=review >>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:192 >>> + adjacentSegments.forEach(function(adjacentSegment) { >> >> Not: Could have also been converted to for (..of..). > > What are our style guidelines with regard to Array.prototype.forEach? I see it being used with and without the optional thisArg. We prefer not to use it now that we have for (..of..). That avoided a lot of function calls and closures.
Comment on attachment 256253 [details] [Patch] Proposed Fix Clearing flags on attachment: 256253 Committed r186387: <http://trac.webkit.org/changeset/186387>
All reviewed patches have been landed. Closing bug.