858927 - Move the mozilla::TimeStamp into mozglue

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Gabriele Svelto [:gsvelto]]

The mozilla::TimeStamp class could be useful outside of the Gecko core. In particular it would allow external code to take timestamps compatible with our internal ones and thus pass them through interfaces such as XRE_StartupTimelineRecord(); see bug 793735 comment 46. Additionally it would allow us to clean up duplicated code such as _PR_Now() in browser/app/nsBrowserApp.cpp:

https://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#392

... and MOZ_Time() in mozglue/android/APKOpen.cpp:

https://mxr.mozilla.org/mozilla-central/source/mozglue/android/APKOpen.cpp#70

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Move the TimeStamp class into MFBT

First patch, tested only on a Linux debug build.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 785362 [details] [diff] [review]
[PATCH WIP] Move the TimeStamp class into MFBT

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

::: mfbt/TimeStamp.cpp
@@ +14,1 @@
>  #include "mozilla/TimeStamp.h"

This header should be included first.

Comment 3

[Mike Hommey [:glandium]]

Please move it in mozglue instead.

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Please move it in mozglue instead.

Will do; I've updated the bug title accordingly.

Comment 5

[Jeff Walden [:Waldo]]

Okay, we need to resolve this mfbt/mozglue confusion now.  I think of mozglue as the place where we put super-low-level stuff like linking goo and allocation stuff.  I think of mfbt as the place where algorithms, data structures, and out-of-line implementations of them go.  So I think mfbt is absolutely the right place for this.  That said, I don't care whether mfbt and mozglue are separate.  Can we just unify the two and have everyone use an mfbtglue thing?

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Okay, we need to resolve this mfbt/mozglue confusion now.  I think of
> mozglue as the place where we put super-low-level stuff like linking goo and
> allocation stuff.  I think of mfbt as the place where algorithms, data
> structures, and out-of-line implementations of them go.  So I think mfbt is
> absolutely the right place for this.

I'm OK both ways; I don't really have any preference for this except that pulling it out will allow me to remove some duplicated ad-hoc code we currently have (see bug 858928) and make all our startup info timestamps fully consistent.

What's a bit special about this code is that it's not really just a template things as there are significant implementation bits in .cpp files and on Linux whoever is including it also needs to link with the 'rt' library. I haven't found a way to implement that part yet in MFBT but I would have done it after verifying the relocated code works properly.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Move the TimeStamp class into MFBT

I haven't had enough time to prepare a patch that moves everything into mozglue yet but I wanted to attach this refreshed patch in the meantime. I should get a chance of finishing this by the end of this week.

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/2] Do not include TimeStamp.h in files that do not use it directly

This is a follow-up I had prepared that cleaned up a few files that are including TimeStamp.h but don't need it directly. I thought it might fit well in this bug and should help with bug 901132 as I've noticed that every time I touch TimeStamp.h it forces half of my tree to be rebuilt. This is a WIP as I've not yet analyzed all files that could be made to not include TimeStamp.h.

I think this should be independent of whether this ends up in MFBT or mozglue.

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 790188 [details] [diff] [review]
[PATCH 2/2] Do not include TimeStamp.h in files that do not use it directly

Obsoleting this as I've moved this part to bug 907798.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: WIP - darwin/& linux-posix working

Comment 12

[Benoit Girard (:BenWa)]

Attachment: WIP - darwin & linux-posix working

Comment 13

[Benoit Girard (:BenWa)]

Attachment: WIP - darwin & linux-posix working

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Some files appear to be missing for the patch, but let me note a preemptive r- for adding anything ns-prefixed into mfbt just to be sure :)

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 16

[Mike Hommey [:glandium]]

To me, this goes well beyond what's supposed to be in mfbt. It should go in mozglue instead (which, you'll note, is what is in the bug title).

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Mike Hommey [:glandium] from comment #16)
> To me, this goes well beyond what's supposed to be in mfbt. It should go in
> mozglue instead (which, you'll note, is what is in the bug title).

Your saying it like it's a resolved fact but Comment 5 still hasn't been answered. (bug title don't necessarily reflect the current flow of the discussion).

https://wiki.mozilla.org/Modules/Core

I don't see Timestamp has low level functionality similar to a dynamic linker but it does fit 'functionality available for use and reuse throughout all Mozilla code (including SpiderMonkey and Gecko more broadly).'.

Can we resolve this properly instead of having me flip-flop the patch?

For my purpose I'm looking to share code with gecko and thus will be embedding MFBT, but I have no interest in pulling in mozglue functionality like a dynamic linker into my standalone project. MFBT makes more sense to me based on my use case and the module description.

Comment 18

[Tom Tromey :tromey]

I don't know anything about mozglue, but in case it matters, for
bug 1159506 I would like to be able to use this from spidermonkey.

Comment 19

[Mike Hommey [:glandium]]

Every time we add a .cpp file to mfbt we're creating a new problem for things using it. As I wrote in the other bug, you can very well have something in mozglue that can _also_ be used independently, that's actually how mfbt is too: usable independently, yet in mozglue.

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Mike Hommey [:glandium] from comment #19)
> Every time we add a .cpp file to mfbt we're creating a new problem for
> things using it.

Why is that?

Comment 21

[Mike Hommey [:glandium]]

(In reply to Benoit Girard (:BenWa) from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > Every time we add a .cpp file to mfbt we're creating a new problem for
> > things using it.
> 
> Why is that?

Because we have things in the tree that statically link mfbt but also load mozglue, which contains mfbt. Fun times.

Comment 22

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch

forgot to hg add the moz.build file.

Comment 24

[Benoit Girard (:BenWa)]

Attachment: patch

I really hate mq.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8617741 [details] [diff] [review]
patch

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

::: mozglue/misc/TimeStamp.h
@@ +40,5 @@
>  public:
> +  static MFBT_API double ToSeconds(int64_t aTicks);
> +  static MFBT_API double ToSecondsSigDigits(int64_t aTicks);
> +  static MFBT_API int64_t TicksFromMilliseconds(double aMilliseconds);
> +  static MFBT_API int64_t ResolutionInTicks();

You can avoid adding MFBT_API for each and every method if you add it to the class itself: class MFBT_API BaseTimeDurationPlatformUtils.

@@ -205,5 @@
>    }
>    BaseTimeDuration operator*(const uint64_t aMultiplier) const
>    {
>      if (aMultiplier > INT64_MAX) {
> -      NS_WARNING("Out-of-range multiplier when multiplying BaseTimeDuration");

Not sure what to think about this. Brian, since you wrote this, any opinion on not warning?

@@ +426,5 @@
>     * lower precision, usually 15.6 ms, but with very good performance benefit.
>     * Use it for measurements of longer times, like >200ms timeouts.
>     */
> +  static MFBT_API TimeStamp Now() { return Now(true); }
> +  static MFBT_API TimeStamp NowLoRes() { return Now(false); }

Same here. The other methods are inline, so they shouldn't be a problem.

::: mozglue/misc/TimeStamp_posix.cpp
@@ +172,4 @@
>  TimeStamp::Startup()
>  {
>    if (gInitialized) {
> +    return true;

If the only result ever returned is NS_OK/true, might as well return void.

@@ +176,5 @@
>    }
>  
>    struct timespec dummy;
>    if (clock_gettime(CLOCK_MONOTONIC, &dummy) != 0) {
> +    MOZ_RELEASE_ASSERT(false, "CLOCK_MONOTONIC is absent!");

You can do MOZ_CRASH("CLOCK_MONOTONIC is absent!");

@@ +293,3 @@
>  
> +  if (pthread_create(&uptime_pthread, nullptr, ComputeProcessUptimeThread, &uptime)) {
> +    MOZ_CRASH("Fail to create thread");

Failed to create process uptime thread.

::: mozglue/misc/TimeStamp_windows.cpp
@@ +508,5 @@
>  
>    InitThresholds();
>    InitResolution();
>  
> +  return true;

Same here, could return void.

::: mozglue/misc/TimeStamp_windows.h
@@ +49,5 @@
>    {
>      return TimeStampValue(mGTC - aOther, mQPC - aOther, mHasQPC);
>    }
> +  MFBT_API TimeStampValue& operator+=(const int64_t aOther);
> +  MFBT_API TimeStampValue& operator-=(const int64_t aOther);

Can probably move MFBT_API to the class declaration here too.

::: xpcom/build/NSPRInterposer.cpp
@@ +9,5 @@
>  
>  #include "prio.h"
>  #include "private/pprio.h"
> +#include "nsDebug.h"
> +#include "nscore.h"

yay unified builds.

Comment 26

[Mike Hommey [:glandium]]

Note, as a followup, you could remove things like https://dxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp?from=timestamp_now&case=true#262
I think we have a couple of those.

Comment 27

[Gabriele Svelto [:gsvelto]]

(In reply to Mike Hommey [:glandium] from comment #26)
> Note, as a followup, you could remove things like
> https://dxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.
> cpp?from=timestamp_now&case=true#262
> I think we have a couple of those.

Yes, see the dependent bug 858928. BTW I don't see the patch linking anywhere to the 'rt' library which is required on Linux to use clock_gettime() and friends. It's possible that the build will work anyway because most users are already linking to that library but I'd rather have it as an explicit dependency so that potential new users won't run into linking issues if they don't add -lrt themselves.

BTW when I first prepared the patch failing to add -lrt caused the automated B2G emulator builds to fail so look out for that.

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8617741 [details] [diff] [review]
patch

(In reply to Mike Hommey [:glandium] from comment #25)
> @@ -205,5 @@
> >    }
> >    BaseTimeDuration operator*(const uint64_t aMultiplier) const
> >    {
> >      if (aMultiplier > INT64_MAX) {
> > -      NS_WARNING("Out-of-range multiplier when multiplying BaseTimeDuration");
> 
> Not sure what to think about this. Brian, since you wrote this, any opinion
> on not warning?

Yeah, that was added at dholbert's suggestion in bug 1016757 comment 3.

It's not the most useful warning (I don't recall ever seeing it) and I think it was a nice-to-have when it was suggested anyway.

Is there any other logging method available in this context we could use? If not, I'm fine with removing this.

Comment 29

[Mike Hommey [:glandium]]

(In reply to Brian Birtles (:birtles) from comment #28)
> Is there any other logging method available in this context we could use? If
> not, I'm fine with removing this.

Unfortunately, not yet.

> BTW I don't see the patch linking anywhere to the 'rt' library which is required on Linux to use clock_gettime() and friends.

Ah indeed, we need OS_LIBS += CONFIG['REALTIME_LIBS'] ; and to make standalone js happy the test for REALTIME_LIBS in configure.in needs to be copied in js/src/configure.in (or moved to a .m4 in build/autoconf)

Comment 30

[Benoit Girard (:BenWa)]

Attachment: patch v2

Carrying forward r+ assuming that the MFBT_API is fine as it. Otherwise we will need to figure out why it doesn't work on the class declaration.

Comment 31

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 32

[Benoit Girard (:BenWa)]

Attachment: patch v3

Moved a simple hunk into this patch for std::getenv -> getenv.

Comment 33

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 858927 - Move the mozilla::TimeStamp into mozglue. r=glandium

Bug 858927 - Move the mozilla::TimeStamp into mozglue. r=glandium

Comment 34

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1172216 - Move nsStackwalk to mozglue. r=glandium

Bug 1172216 - Move nsStackwalk to mozglue. r=glandium

Comment 35

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1172186 - Make the profiler build standalone. r=mstange

Bug 1172186 - Make the profiler build standalone. r=mstange

Comment 36

[Benoit Girard (:BenWa)]

Comment on attachment 8623970 [details]
MozReview Request: Bug 1172186 - Make the profiler build standalone. r=mstange

Bug 1172186 - Make the profiler build standalone. r=mstange

Comment 37

[Benoit Girard (:BenWa)]

Comment on attachment 8623968 [details]
MozReview Request: Bug 858927 - Move the mozilla::TimeStamp into mozglue. r=glandium

Bug 858927 - Move the mozilla::TimeStamp into mozglue. r=glandium

Comment 38

[Benoit Girard (:BenWa)]

Comment on attachment 8623969 [details]
MozReview Request: Bug 1172216 - Move nsStackwalk to mozglue. r=glandium

Bug 1172216 - Move nsStackwalk to mozglue. r=glandium

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 8623970 [details]
MozReview Request: Bug 1172186 - Make the profiler build standalone. r=mstange

Bug 1172186 - Make the profiler build standalone. r=mstange

Comment 40

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2f1318c3c0

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf2f1318c3c0