Bug 80786 - Web Inspector: use monotonically increasing time in timeline agent
Summary: Web Inspector: use monotonically increasing time in timeline agent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks: 79401
  Show dependency treegraph
 
Reported: 2012-03-11 05:34 PDT by Andrey Kosyakov
Modified: 2012-03-11 11:09 PDT (History)
14 users (show)

See Also:


Attachments
Patch (6.62 KB, patch)
2012-03-11 05:37 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2012-03-11 06:05 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (7.20 KB, patch)
2012-03-11 06:33 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (11.47 KB, patch)
2012-03-11 08:36 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-03-11 05:34:22 PDT
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.
Comment 1 Andrey Kosyakov 2012-03-11 05:37:46 PDT
Created attachment 131217 [details]
Patch
Comment 2 Kentaro Hara 2012-03-11 05:46:56 PDT
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).
Comment 3 Pavel Feldman 2012-03-11 05:56:07 PDT
Comment on attachment 131217 [details]
Patch

I guess Kentaro means r-.
Comment 4 Andrey Kosyakov 2012-03-11 06:05:50 PDT
Created attachment 131219 [details]
Patch
Comment 5 Kentaro Hara 2012-03-11 06:07:51 PDT
Comment on attachment 131219 [details]
Patch

Looks OK!
Comment 6 Pavel Feldman 2012-03-11 06:17:31 PDT
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.
Comment 7 Andrey Kosyakov 2012-03-11 06:33:22 PDT
Created attachment 131220 [details]
Patch
Comment 8 Andrey Kosyakov 2012-03-11 06:37:19 PDT
(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.
Comment 9 Pavel Feldman 2012-03-11 07:27:51 PDT
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?
Comment 10 Yury Semikhatsky 2012-03-11 07:29:32 PDT
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?
Comment 11 Andrey Kosyakov 2012-03-11 08:36:31 PDT
Created attachment 131227 [details]
Patch
Comment 12 Andrey Kosyakov 2012-03-11 09:36:33 PDT
Committed r110394: <http://trac.webkit.org/changeset/110394>
Comment 13 Timothy Hatcher 2012-03-11 10:22:53 PDT
Comment on attachment 131227 [details]
Patch

From bug 79401: Ideally this would have used DocumentLoadTiming::convertMonotonicTimeToDocumentTime to have the same base timestamp as resources.
Comment 14 Timothy Hatcher 2012-03-11 10:26:09 PDT
(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.
Comment 15 Timothy Hatcher 2012-03-11 10:27:35 PDT
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.
Comment 16 Andrey Kosyakov 2012-03-11 10:32:28 PDT
> 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).
Comment 17 Timothy Hatcher 2012-03-11 11:09:42 PDT
(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!