858928 - Switch XRE_StartupTimelineRecord() from PRTime to mozilla::TimeStamp

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

After bug 793735 will have landed it would be good to switch XRE_StartupTimelineRecord() to use the TimeStamp class in order to have end-to-end compatible timestamps in the startup timeline. Changing the function signature from PRTime to TimeStamp will also require fixing up the current users of this function, namely browser/app/nsBrowserApp.cpp and mozglue/android/APKOpen.cpp.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Switch XRE_StartupTimelineRecord() from PRTime to TimeStamp

Now that TimeStamp is in mozglue we can finally remove these 100+ lines of redundant code. The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cff5b63a2187

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Switch XRE_StartupTimelineRecord() from PRTime to TimeStamp

Whoops, I attached the wrong patch, this is the right one.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Switch XRE_StartupTimelineRecord() from PRTime to TimeStamp

It turns out that the Android build was broken, I've updated the patch to fix this issue.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8626042 [details] [diff] [review]
[PATCH v2] Switch XRE_StartupTimelineRecord() from PRTime to TimeStamp

Review of attachment 8626042 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for deleted code.

::: mozglue/android/moz.build
@@ +30,5 @@
>  
>  GENERATED_INCLUDES += ['/build']
>  LOCAL_INCLUDES += [
>      '../linker',
> +    '../misc',

I assume this is to pick up TimeStamp.h?  Can't we just get it by including mozilla/TimeStamp.h in APKOpen.cpp instead of bare TimeStamp.h?

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> I assume this is to pick up TimeStamp.h?  Can't we just get it by including
> mozilla/TimeStamp.h in APKOpen.cpp instead of bare TimeStamp.h?

I'm not sure, I can try, I just went along with what the code did with the rest of the headers :-)

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

I've just tried using mozilla/TimeStamp.h so that I don't have to add misc to the local includes, this seems to work across all of our architectures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab357f02d24a

Shall I land the patch this way then?

Comment 7

[Nathan Froyd [:froydnj]]

Yes!

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Switch XRE_StartupTimelineRecord() from PRTime to TimeStamp

Final version of the patch addressing the review comment in comment 4, carrying over the r+ flag.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Time to land, the green try run for patch v3 is this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab357f02d24a

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9c005e6798

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1e9c005e6798

Comment 12

[(Away)]

A surprise side effect of this change is that mozglue.dll (and in turn msvcr120.dll) is now loaded slightly earlier in startup. Previously the load was caused by DllBlocklist_Initialize() and now it is caused by TimeStamp::Now(). I *think* this is okay, but there is a very delicate balance in the startup sequence on Windows, so keep an eye out for regressions.