Bug 98223

Created attachment 166793 [details] Patch

Comment on attachment 166793 [details] Patch Attachment 166793 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14125541

Created attachment 166799 [details] Patch

This means we don't need the ugly conversion code and should make the times more accurate too.

Comment on attachment 166799 [details] Patch Attachment 166799 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14138427 New failing tests: http/tests/inspector/resource-har-conversion.html http/tests/inspector/resource-parameters.html

Comment on attachment 166799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166799&action=review > Source/WebCore/inspector/InspectorInstrumentation.cpp:-746 > - double finishTime = 0.0; why did this change? > Source/WebCore/inspector/InspectorPageAgent.cpp:1007 > + m_geolocationPosition = GeolocationPosition::create(monotonicallyIncreasingTime() * 1000.0, *latitude, *longitude, *accuracy); I am not sure what this time stands for, it might be accessible via the web facing API, we might want to have wall time here. > Source/WebCore/inspector/InspectorResourceAgent.cpp:115 > + .setRequestTime(timing.requestTime) Is there a corresponding downstream change? > Source/WebCore/inspector/front-end/CanvasProfileView.js:145 > + this._debugInfoElement.textContent = "Replay time: " + (performance.webkitNow() - time) + "ms"; Here and below: why do you change this? it does not operate backend timers, so i'd leave it as is. otherwise we would need to remove prefix at some point.

(In reply to comment #6) > (From update of attachment 166799 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166799&action=review > > > Source/WebCore/inspector/InspectorInstrumentation.cpp:-746 > > - double finishTime = 0.0; > > why did this change? Previously, we'd convert the monotonicFinishTime (passed in to this function) to a currentTime-like time by calling convertMonotonicTimeToDocumentTime. Now that I've changed inspector to always use monotonic times, this is no longer necessary, so we just pass the (monotonic) finishTime through without modification. > > Source/WebCore/inspector/InspectorPageAgent.cpp:1007 > > + m_geolocationPosition = GeolocationPosition::create(monotonicallyIncreasingTime() * 1000.0, *latitude, *longitude, *accuracy); > > I am not sure what this time stands for, it might be accessible via the web facing API, we might want to have wall time here. Fair enough. Done. > > Source/WebCore/inspector/InspectorResourceAgent.cpp:115 > > + .setRequestTime(timing.requestTime) > > Is there a corresponding downstream change? No, it's always been a monotonic time. We're just passing it through instead of converting it to a pseudo-currentTime (see my comment earlier about finishTime). > > Source/WebCore/inspector/front-end/CanvasProfileView.js:145 > > + this._debugInfoElement.textContent = "Replay time: " + (performance.webkitNow() - time) + "ms"; > > Here and below: why do you change this? it does not operate backend timers, so i'd leave it as is. otherwise we would need to remove prefix at some point. currentTime() (aka Date.now()) is susceptible to the user's clock changing, either manually or through NTP. It's strictly better to use monotonicallyIncreasingTime (aka performance.webkitNow()) when timing operations. However, Tony pointed out off-list that the mac port doesn't support webkitNow. So, I'll revert the JS changes for now. We should switch back ASAP. (In reply to comment #5) > (From update of attachment 166799 [details]) > Attachment 166799 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14138427 > > New failing tests: > http/tests/inspector/resource-har-conversion.html > http/tests/inspector/resource-parameters.html So HAR actually has a use for currentTime. The startedDateTime must be a Date. I still think it's the right thing to use monotonic time everywhere in inspector for the reasons listed above. I'd like to treat HAR a special case and let it add in the offset as needed, but leave the rest of the code only dealing with monotonic times. I've attached a patch that does that, but it feels clunky. If any of you inspector gurus have a better approach, please let me know.

Created attachment 167025 [details] Patch

Comment on attachment 167025 [details] Patch Attachment 167025 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14172040 New failing tests: http/tests/inspector/resource-har-pages.html

Created attachment 167889 [details] Patch

> (In reply to comment #5) > > (From update of attachment 166799 [details] [details]) > > Attachment 166799 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/14138427 > > > > New failing tests: > > http/tests/inspector/resource-har-conversion.html > > http/tests/inspector/resource-parameters.html > > So HAR actually has a use for currentTime. The startedDateTime must be a Date. > > I still think it's the right thing to use monotonic time everywhere in inspector for the reasons listed above. I'd like to treat HAR a special case and let it add in the offset as needed, but leave the rest of the code only dealing with monotonic times. > > I've attached a patch that does that, but it feels clunky. If any of you inspector gurus have a better approach, please let me know. Ping? I'd love some feedback from the inspector team about how to handle the HAR writing code.

Comment on attachment 167889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167889&action=review I think we save few lines of code and it becomes more straightforward from the maintenance standpoint, but at the same time, I think naive protocol users will not appreciate the getMonotonicClockOffset necessity. What is the main motivation behind this change again? > Source/WebCore/inspector/Inspector.json:926 > + "name": "getMonotonicClockOffset", How often do you anticipate clients to call this? I.e. this value can change at chrome runtime, right? > Source/WebCore/inspector/InspectorCSSAgent.cpp:181 > + m_currentMatchData.startTime = monotonicallyIncreasingTime() * 1000.0; I'd convert domains one by one. For example, converting CSS profiler in a separate change would make more sense to me. > Source/WebCore/inspector/front-end/HAREntry.js:324 > + monotonicStartTime: page.startTime, I don't think HAR spec has this field? http://www.softwareishard.com/blog/har-12-spec/

(In reply to comment #12) > (From update of attachment 167889 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167889&action=review > > I think we save few lines of code and it becomes more straightforward from the maintenance standpoint, but at the same time, I think naive protocol users will not appreciate the getMonotonicClockOffset necessity. What is the main motivation behind this change again? The ResourceLoadTiming struct that comes from the embedder contains monotonic clock values (TimeTicks in Chrome). However, inspector currently uses currentTime() for everything. That means we need to convert those monotonic times to wall times before they can be used in inspector. Using wall times is bad for inspector. If the user's clock shifts (most likely due to background clock synchronization), then any duration reported by inspector is bogus. So, by switching to monotonic times, we can get rid of this conversion layer and improve the accuracy of our time measurement. The one exception is HAR's startedDateTime, which is spec'd to be a wall time. For that, we need to do one conversion. > > > Source/WebCore/inspector/Inspector.json:926 > > + "name": "getMonotonicClockOffset", > > How often do you anticipate clients to call this? I.e. this value can change at chrome runtime, right? It definitely can change. My opinion is that the startedDateTime doesn't need to be super accurate, so only updating it once, or very rarely, is good enough for me. I was hoping the HAR writer would be the only client for this function. Is there any way to restrict it to that? Another option is to have getMonotonicallyIncreasingTime() instead of getMonotonicClockOffset(). The HAR writer could compute its own offset then. > > > Source/WebCore/inspector/InspectorCSSAgent.cpp:181 > > + m_currentMatchData.startTime = monotonicallyIncreasingTime() * 1000.0; > > I'd convert domains one by one. For example, converting CSS profiler in a separate change would make more sense to me. None of the time values have distinct types, so it's hard to know which ones use the old format and which use the new format. I think switching all of them at once would avoid any confusion. > > Source/WebCore/inspector/front-end/HAREntry.js:324 > > + monotonicStartTime: page.startTime, > > I don't think HAR spec has this field? http://www.softwareishard.com/blog/har-12-spec/ It's a hack. It's not actually exposed. I'll come up with a better way of accomplishing this. I'm thinking of having a HAREntryFactory that stores the offset. I'll try to post a better patch tomorrow.