Bug 80786 – Web Inspector: use monotonically increasing time in timeline agent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

This switches timeline agent to use monotonically increasing time, so timeline records are not affected by clock adjustments. Units remain the same, i.e. fractional milliseconds.
We may also consider using times since record start.

[Andrey Kosyakov]

Created attachment 131217 [details]
Patch

[Kentaro Hara]

Comment on attachment 131217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131217&action=review

> Source/WebCore/inspector/InspectorTimelineAgent.h:160
> +    static double timestamp();
> +    static double timestamp(double microseconds) { return microseconds * 1000.0; }

I am not super excited at this overloading. timestamp() and timestamp(double) are doing different things. Maybe we can rename timestamp(double) to timestampToMilliseconds(double).

[Pavel Feldman]

Comment on attachment 131217 [details]
Patch

I guess Kentaro means r-.

[Andrey Kosyakov]

Created attachment 131219 [details]
Patch

[Kentaro Hara]

Comment on attachment 131219 [details]
Patch

Looks OK!

[Pavel Feldman]

Comment on attachment 131219 [details]
Patch

Kentaro, could you please avoid reviewing (r+ -ing) inspector changes without knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing.

[Andrey Kosyakov]

Created attachment 131220 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #6)
> (From update of attachment 131219 [details])
knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing.

I'm now using re-basing monotonically increasing ticks so that they match current time as of record start -- so these timestamps may be compared to time provided by other agents (e.g. network), as long as there were no time adjustments or suspension. We probably need to switch to monotonically increasing time in network as well.

[Pavel Feldman]

Comment on attachment 131220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131220&action=review

Looks good with a nit. Could you please add a test?

> Source/WebCore/inspector/InspectorTimelineAgent.h:160
> +    double timestampFromMicroseconds(double microseconds) { return (microseconds + m_timestampOffset) * 1000.0; }

Why is it implemented in header?

[Yury Semikhatsky]

View in context: https://bugs.webkit.org/attachment.cgi?id=131220&action=review

> Source/WebCore/bindings/v8/ScriptGCEvent.cpp:93
> +    s_startTime = WTF::monotonicallyIncreasingTime();

This variable should be per isolate. Could you file a bug on this?

[Andrey Kosyakov]

Created attachment 131227 [details]
Patch

[Andrey Kosyakov]

Committed r110394: <http://trac.webkit.org/changeset/110394>

[Timothy Hatcher]

Comment on attachment 131227 [details]
Patch

From bug 79401: Ideally this would have used DocumentLoadTiming::convertMonotonicTimeToDocumentTime to have the same base timestamp as resources.

[Timothy Hatcher]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 131219 [details] [details])
> knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing.
> 
> I'm now using re-basing monotonically increasing ticks so that they match current time as of record start -- so these timestamps may be compared to time provided by other agents (e.g. network), as long as there were no time adjustments or suspension. We probably need to switch to monotonically increasing time in network as well.

Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime.

[Timothy Hatcher]

Also it would have been nice to search the Inspector bug list for "monotonically" before filing a new bug, or you would have found bug 79401.

[Andrey Kosyakov]

> Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime.

I was just about to comment on this in bug 79401. There must be some confustion there:

$ grep -c monotonically InspectorResourceAgent.cpp 
0
$ grep -c currentTime InspectorResourceAgent.cpp 
9

>  "monotonically" before filing a new bug, or you would have found bug 79401.

Fair enough (this came as an afterthought, and this is exactly how I found it).

[Timothy Hatcher]

(In reply to comment #16)
> > Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime.
> 
> I was just about to comment on this in bug 79401. There must be some confustion there:
> 
> $ grep -c monotonically InspectorResourceAgent.cpp 
> 0
> $ grep -c currentTime InspectorResourceAgent.cpp 
> 9

You're right. I was seeing the code in InspectorResourceAgent::buildObjectForTiming and got the idea it already used it through out. Looks like it is mixed!