STEPS TO REPRODUCE: 1. Start recording on some page that allocates and frees memory predictably 2. Stop the recording, then do something that frees up memory (without navigating the page) 3. Start the recording again. EXPECTED: Like the timeline bars, the memory graph shouldn't render anything during the interval that was not recorded. ACTUAL: The line segment doesn't skip, so it appears to interpolate the line between two sides of the gap.
Created attachment 279737 [details] Screenshot
Tim and I were discussing this the other day. We were planning to wait until we had a better way to show gaps in the recording (a jagged "break" or something similar), but that could be a ways out. I like the idea of showing breaks.
(In reply to comment #2) > Tim and I were discussing this the other day. We were planning to wait until > we had a better way to show gaps in the recording (a jagged "break" or > something similar), but that could be a ways out. I like the idea of showing > breaks. Since we don't yet support discontinuity in the ruler (i.e., 1-5s, jaggy, then 10-15s) there's no reason to wait for that to happen. This should be a simple fix of adding zero points to the path for recording stop/restart.
(In reply to comment #3) > (In reply to comment #2) > > Tim and I were discussing this the other day. We were planning to wait until > > we had a better way to show gaps in the recording (a jagged "break" or > > something similar), but that could be a ways out. I like the idea of showing > > breaks. > > Since we don't yet support discontinuity in the ruler (i.e., 1-5s, jaggy, > then 10-15s) there's no reason to wait for that to happen. This should be a > simple fix of adding zero points to the path for recording stop/restart. I have it mostly working, will clean up and post later this evening.
Ping for import.
<rdar://problem/26516695>
Created attachment 280048 [details] [Image] New UI
Created attachment 280049 [details] [Patch] Proposed Fix
Comment on attachment 280049 [details] [Patch] Proposed Fix I am not awake enough to review. Eventually, I think we may want to create discrete 'segments' in the recording, like audio tracks. That will make it possible to graph the data as (segment offset + record start).
Comment on attachment 280049 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280049&action=review > Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:297 > + for (let discontinuity of this._discontinuities) { Is there a reason why you couldn't perform a filter operation on this._discontinuities? return this._discontinuities.filter(item => item.startTime < endTime && item.endTime > startTime);
Comment on attachment 280049 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280049&action=review >> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:297 >> + for (let discontinuity of this._discontinuities) { > > Is there a reason why you couldn't perform a filter operation on this._discontinuities? > > return this._discontinuities.filter(item => item.startTime < endTime && item.endTime > startTime); Nice style change. Filter is much more concise.
(In reply to comment #9) > Comment on attachment 280049 [details] > [Patch] Proposed Fix > > I am not awake enough to review. Eventually, I think we may want to create > discrete 'segments' in the recording, like audio tracks. That will make it > possible to graph the data as (segment offset + record start). Agreed. As it is, we're drawing a single SVG for all segments, resulting in more cumbersome code and a noticeable line running along the x-axis connecting all the segments.
Created attachment 280066 [details] [Patch] Proposed Fix
(In reply to comment #12) > (In reply to comment #9) > > Comment on attachment 280049 [details] > > [Patch] Proposed Fix > > > > I am not awake enough to review. Eventually, I think we may want to create > > discrete 'segments' in the recording, like audio tracks. That will make it > > possible to graph the data as (segment offset + record start). > > Agreed. As it is, we're drawing a single SVG for all segments, resulting in > more cumbersome code and a noticeable line running along the x-axis > connecting all the segments. Filed follow-up https://bugs.webkit.org/show_bug.cgi?id=158195.
Comment on attachment 280066 [details] [Patch] Proposed Fix Attachment 280066 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1403797 Number of test failures exceeded the failure limit.
Created attachment 280067 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Does this work from all views or just Memory? It would be weird to only do this for Memory.
(In reply to comment #17) > Does this work from all views or just Memory? It would be weird to only do > this for Memory. I think Memory is the only view that has this issue, since it is the only view that draws content from record to record. Bubbles (JS + Layout) do not extend beyond their own bounds. Maybe this could affect networking events with long tails, but network events aren't really discontinuous because we have complete data even if the timeline wasn't recording.
Comment on attachment 280066 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280066&action=review r=me, nicely done! > Source/WebInspectorUI/UserInterface/Models/Timeline.js:106 > + visibleRecords(startTime, endTime, includeRecordBeforeStart) The word "visible" really doesn't make sense anymore on the Model object. I like "recordsInTimeRange" much better. What do you think? > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224 > + console.assert(categoryData.dataPoints.length); > + let previousDataPoint = categoryData.dataPoints.lastValue; Hmm, I worry that we might encounter something where there is not a previous data point. What happens in this scenario: 1. Start at recording with just the Scripts timeline. 2. Stop the recording. 3. Add the Memory timeline. 4. Start recording. 5. Stop recording. 6. Select Memory Timeline. 7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline. The same goes to the Overview Graph code, but that has and assert and early return. > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:232 > + categoryData.dataPoints.push({time: discontinuity.startTime, size: previousDataPoint.size}); > + > + categoryData.dataPoints.push({time: discontinuity.startTime, size: 0}); > + categoryData.dataPoints.push({time: discontinuity.endTime, size: 0}); > + > + categoryData.dataPoints.push({time: discontinuity.endTime, size: category.size}); Style: I think you can drop the comment an newlines. The code is very clean and concise!
Comment on attachment 280066 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280066&action=review >> Source/WebInspectorUI/UserInterface/Models/Timeline.js:106 >> + visibleRecords(startTime, endTime, includeRecordBeforeStart) > > The word "visible" really doesn't make sense anymore on the Model object. I like "recordsInTimeRange" much better. What do you think? Agree. I really like "recordsInTimeRange". I think visibleRecords is still okay at call sites for the returned value, since "visible" makes sense with the additional context. >> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224 >> + let previousDataPoint = categoryData.dataPoints.lastValue; > > Hmm, I worry that we might encounter something where there is not a previous data point. > > What happens in this scenario: > > 1. Start at recording with just the Scripts timeline. > 2. Stop the recording. > 3. Add the Memory timeline. > 4. Start recording. > 5. Stop recording. > 6. Select Memory Timeline. > 7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline. > > The same goes to the Overview Graph code, but that has and assert and early return. Great catch! TypeError: undefined is not an object (evaluating 'previousDataPoint.size') (at MemoryTimelineView.js:227:105). >> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:232 >> + categoryData.dataPoints.push({time: discontinuity.endTime, size: category.size}); > > Style: I think you can drop the comment an newlines. The code is very clean and concise! Always happy to drop newlines.
Comment on attachment 280066 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280066&action=review >>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224 >>> + let previousDataPoint = categoryData.dataPoints.lastValue; >> >> Hmm, I worry that we might encounter something where there is not a previous data point. >> >> What happens in this scenario: >> >> 1. Start at recording with just the Scripts timeline. >> 2. Stop the recording. >> 3. Add the Memory timeline. >> 4. Start recording. >> 5. Stop recording. >> 6. Select Memory Timeline. >> 7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline. >> >> The same goes to the Overview Graph code, but that has and assert and early return. > > Great catch! TypeError: undefined is not an object (evaluating 'previousDataPoint.size') (at MemoryTimelineView.js:227:105). We can also do better in the overview graph. Currently discontinuities before the first data point aren't shown.
Created attachment 280519 [details] [Patch] Proposed Fix
Fixed an issue with the category charts in MemoryTimelineView, that would cause the chart to extend past gaps at the beginning and end of the graph.
Comment on attachment 280519 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=280519&action=review R=me > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:243 > + // If the graph end time is inside a gap, the last data point should > + // only be extended to the start of the discontinuity. > + if (discontinuities.length) > + visibleEndTime = discontinuities[0].startTime; Nice!
Comment on attachment 280519 [details] [Patch] Proposed Fix Clearing flags on attachment: 280519 Committed r201686: <http://trac.webkit.org/changeset/201686>
All reviewed patches have been landed. Closing bug.