1442746 - Use clock_gettime on BSDs too in Telemetry ?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Landry Breuil (:gaston)]

Since 9 days m-c fails to build on OpenBSD (with ldd, & disable-webkit) for:

error: undefined symbol: std::__1::chrono::system_clock::now()
145:56.71 >>> referenced by Telemetry.cpp:1656 (/build/buildslave-amd64/mozilla-central-amd64/build/toolkit/components/telemetry/Telemetry.cpp:1656)
145:56.71 >>>               ../components/telemetry/Telemetry.o:((anonymous namespace)::TelemetryImpl::MsSystemNow(double*))

This code was added in bug 1352961 a long while ago but was only built/enabled recently, as 994a684a7564c2735d98d6910a78d079a68f0b25 was green (cf http://buildbot.rhaalovely.net/nine/#/builders/3/builds/51) but turned red with 169b1ba48437d5ddc0a4a1828177770b598f7c05 (cf http://buildbot.rhaalovely.net/nine/#/builders/3/builds/52)

My wild guess would be using the clock_gettime #ifdef on BSDs too would fix it. Will try that locally.

Comment 1

[Landry Breuil (:gaston)]

Jan, rings a bell ?

Comment 2

[Jan Beich]

Attachment: Bug 1442746 - Explicitly mark <chrono> as system header.

On libc++ systems before e28a37787c93 <chrono> was bootlegged with
"default" visibility via <fstream>.

In file included from toolkit/components/telemetry/Telemetry.cpp:9:
In file included from objdir/dist/system_wrappers/fstream:3:
In file included from /usr/include/c++/v1/fstream:169:
In file included from objdir/dist/stl_wrappers/ostream:44:
In file included from objdir/dist/system_wrappers/ostream:3:
In file included from /usr/include/c++/v1/ostream:138:
In file included from objdir/dist/stl_wrappers/ios:44:
In file included from objdir/dist/system_wrappers/ios:3:
In file included from /usr/include/c++/v1/ios:216:
In file included from /usr/include/c++/v1/__locale:18:
In file included from /usr/include/c++/v1/mutex:189:
In file included from /usr/include/c++/v1/__mutex_base:15:
/usr/include/c++/v1/chrono:1059:2: fatal error: #error test

Review commit: https://reviewboard.mozilla.org/r/224766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224766/

Comment 3

[Landry Breuil (:gaston)]

Wouldnt it make more sense to use clock_gettime() ?

Comment 4

[Landry Breuil (:gaston)]

Attachment: use clock gettime on unix

My take would be to replace XP_LINUX by XP_UNIX, works fine here.

Comment 5

[Jan Beich]

Comment on attachment 8955900 [details] [diff] [review]
use clock gettime on unix

Did OS X finally implement clock_gettime()? I mean, it's in POSIX and OS X is a certified UNIX...

http://pubs.opengroup.org/onlinepubs/9699919799/functions/clock_getres.html

Comment 6

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #3)
> Wouldnt it make more sense to use clock_gettime() ?

No. According to bug 1352961 comment 24 it's only an issue for --enable-stdcxx-compat.

I'm more puzzled why msSystemNow isn't steady_clock or CLOCK_MONOTONIC. Existing consumers mainly measure delay but clock drifts, DST changes, etc. will add noise to the data.

Comment 7

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #6)
> (In reply to Landry Breuil (:gaston) from comment #3)
> > Wouldnt it make more sense to use clock_gettime() ?
> 
> No. According to bug 1352961 comment 24 it's only an issue for
> --enable-stdcxx-compat.
> 
> I'm more puzzled why msSystemNow isn't steady_clock or CLOCK_MONOTONIC.
> Existing consumers mainly measure delay but clock drifts, DST changes, etc.
> will add noise to the data.

Well then disregard my patch - as long as it's fixed one way or the other... i'll test yours.

Comment 8

[Chris H-C :chutten]

(In reply to Jan Beich from comment #6)
> (In reply to Landry Breuil (:gaston) from comment #3)
> > Wouldnt it make more sense to use clock_gettime() ?
> 
> No. According to bug 1352961 comment 24 it's only an issue for
> --enable-stdcxx-compat.
> 
> I'm more puzzled why msSystemNow isn't steady_clock or CLOCK_MONOTONIC.
> Existing consumers mainly measure delay but clock drifts, DST changes, etc.
> will add noise to the data.

Are steady_clock/CLOCK_MONOTONIC usable across process boundaries? To my recollection, the reason gabor had to use a client clock was because the monotonic ones were tied to process uptime. (Though my recollection is fuzzy. I was only r? because the code was augmenting Telemetry)

Comment 9

[Chris H-C :chutten]

Comment on attachment 8955900 [details] [diff] [review]
use clock gettime on unix

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

Could you quantify the impact (if any) you expect when OSX switches to using clock_gettime? (a clean try run would also help assuage my initial fears)

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8955679 [details]
Bug 1442746 - Explicitly mark <chrono> as system header.

Does this actually fix the problems noted in bug 1352961 comment 24?  Oh, wait, I see, we don't use <chrono> on Linux because of the problems there.

Can we just make the *BSDs use the Linux code path...?

Comment 11

[Jan Beich]

(In reply to Chris H-C :chutten from comment #8)
> Are steady_clock/CLOCK_MONOTONIC usable across process boundaries? To my
> recollection, the reason gabor had to use a client clock was because the
> monotonic ones were tied to process uptime. (Though my recollection is
> fuzzy. I was only r? because the code was augmenting Telemetry)

steady_clock is tied to system uptime (i.e., CLOCK_MONOTONIC), not process uptime (i.e., CLOCK_PROCESS_CPUTIME_ID). Bug 1352961 comment 3 doesn't go into details why monotonic Epoch may change. For one, network and cold reboots are such boundaries but why would Firefox send IPC messages across them. Maybe "usability" wasn't about de facto but rather "guarantee" from the standard (C++11 or POSIX) "an unspecified point in the past" outlives all processes.

(In reply to Nathan Froyd [:froydnj] from comment #10)
> Does this actually fix the problems noted in bug 1352961 comment 24?

--enable-stdcxx-compat makes no sense for from-source builds downstream. On libc++ platforms like FreeBSD, OpenBSD or OS X libstdc++ hacks are nop. IIUC, libc++ only supports C++11 ABI while -std=c++98 uses C++11 surrogates (e.g., nullptr, decltype).

> Can we just make the *BSDs use the Linux code path...?

Like #if defined(XP_UNIX) && !defined(XP_MACOSX) ?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Jan Beich from comment #11)
> (In reply to Nathan Froyd [:froydnj] from comment #10)
> > Can we just make the *BSDs use the Linux code path...?
> 
> Like #if defined(XP_UNIX) && !defined(XP_MACOSX) ?

Or #if defined(HAVE_CLOCK_MONOTONIC), perhaps.

Comment 13

[Jan Beich]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Jan Beich from comment #11)
> > (In reply to Nathan Froyd [:froydnj] from comment #10)
> > > Can we just make the *BSDs use the Linux code path...?
> > 
> > Like #if defined(XP_UNIX) && !defined(XP_MACOSX) ?
> 
> Or #if defined(HAVE_CLOCK_MONOTONIC), perhaps.

But the code uses CLOCK_REALTIME, not CLOCK_MONOTONIC.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Jan Beich from comment #13)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > (In reply to Jan Beich from comment #11)
> > > (In reply to Nathan Froyd [:froydnj] from comment #10)
> > > > Can we just make the *BSDs use the Linux code path...?
> > > 
> > > Like #if defined(XP_UNIX) && !defined(XP_MACOSX) ?
> > 
> > Or #if defined(HAVE_CLOCK_MONOTONIC), perhaps.
> 
> But the code uses CLOCK_REALTIME, not CLOCK_MONOTONIC.

...that doesn't make a lot of sense to me, but OK.

Comment 15

[Landry Breuil (:gaston)]

so... in the end, which way should it be fixed ? m-c is busted on bsds for various bugs, and it would be nice to entangle all of them to have a clean state for the 60 cycle.

Comment 16

[Chris H-C :chutten]

I will r+ a change that expands from XP_LINUX to (XP_UNIX && !XP_DARWIN) (see https://wiki.mozilla.org/Platform/Platform-specific_build_defines for a helpful chart)

Comment 17

[Landry Breuil (:gaston)]

Attachment: use clock_gettime on unix (but not osx)

i dont think i have try access anymore (or dont really remember how to run it, it's been a while) but this patch should be tested on linux/osx ::)

Comment 18

[Chris H-C :chutten]

Comment on attachment 8957316 [details] [diff] [review]
use clock_gettime on unix (but not osx)

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

Whitespace nits. I'll throw this up on try in a minute.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +7,5 @@
>  #include <algorithm>
>  
>  #include <prio.h>
>  #include <prproces.h>
> +#if defined(XP_UNIX) && ! defined (XP_DARWIN)

There shouldn't be whitespace between ! and defined, and defined and (. See https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/js/src/ds/MemoryProtectionExceptionHandler.cpp#14

@@ +1646,5 @@
>  
>  NS_IMETHODIMP
>  TelemetryImpl::MsSystemNow(double* aResult)
>  {
> +#if defined(XP_UNIX) && ! defined (XP_DARWIN)

ditto

Comment 19

[Chris H-C :chutten]

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

Comment 20

[Landry Breuil (:gaston)]

Well, seems all green to me.

Comment 21

[Landry Breuil (:gaston)]

Attachment: use clock_gettime on unix (but not osx)

whitespaces fixed

Comment 22

[Chris H-C :chutten]

Comment on attachment 8957453 [details] [diff] [review]
use clock_gettime on unix (but not osx)

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

LGTM. Would you like me to mark this for checkin, or shall I leave it to you?

Comment 23

[Landry Breuil (:gaston)]

I'm not well-versed in autoland, so i'll let the sherrifs land it :)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/865c06c8aae7
use clock_gettime on all unices but osx, not only linux - fixes build on BSD. r=chutten

Comment 25

[Dorel Luca [:dluca]]

Backed out changeset 865c06c8aae7 (bug 1442746) for lint failure on gecko/toolkit/components/telemetry/parse_scalars.py

Log of the failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca5b27cbe3124c113c433de15e6a2e383678a30

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca5b27cbe3124c113c433de15e6a2e383678a30

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=865c06c8aae729a258d9f56d4f292421ecdfae57

Comment 26

[Landry Breuil (:gaston)]

    TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:62:86 | statement ends with a semicolon (E703)
    TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:69:100 | line too long (107 > 99 characters) (E501)
    TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:75:100 | line too long (108 > 99 characters) (E501)


Sorry, but that makes absolutely no sense to me. You want to backout 13163586e52b from Bug 1426461.

Comment 27

[Nathan Froyd [:froydnj]]

(In reply to Landry Breuil (:gaston) from comment #26)
> Sorry, but that makes absolutely no sense to me. You want to backout
> 13163586e52b from Bug 1426461.

Clearly it makes some sense, otherwise you wouldn't have suggestions on what else to backout! ;)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ffe013db751
use clock_gettime on all unices but osx, not only linux - fixes build on BSD. r=chutten

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4ffe013db751