Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac clock shim not needed after 10.12 #937

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

cellularmitosis
Copy link
Contributor

@cellularmitosis cellularmitosis commented Mar 22, 2022

Summary

This PR:

  • changes the Mac clock shim to only be used for Macs older than 10.12.
  • moves the AvailabilityMacros.h include from os.c to util.c
  • removes the unused mach/clock.h and mach/mach.h includes from os.c

Details

In util.c, the Mac clock shim was included for all Macs:

if defined(JANET_APPLE)

However, clock_gettime() was added in 10.12, so the clock shim isn't needed from that point forward:

This PR changes the preprocessor logic to only use the clock shim for Macs running OS X older than 10.12:

/* clock_gettime() wasn't available on Mac until 10.12. */
#elif defined(JANET_APPLE) && !defined(MAC_OS_X_VERSION_10_12)

Update: I also noticed AvailabilityMacros.h was included in os.c, but is only used in util.c, so I moved the include.

Update2: Also noticed that mach/clock.h and mach/mach.h were included in os.c, but are only needed for the shim. I'd guess that the clock shim stuff used to live in os.c before being pulled out into util.c.

@cellularmitosis cellularmitosis changed the title Mac clock shim not needed until 10.12 Mac clock shim not needed after 10.12 Mar 22, 2022
@cellularmitosis
Copy link
Contributor Author

cellularmitosis commented Mar 22, 2022

It looks like clock_gettime is about 100 times faster than the shim on my M1 running BigSur 11.6.

bench1.c:

#include <time.h>
#include <stdio.h>

int janet_gettime(struct timespec *spec) {
    return clock_gettime(CLOCK_REALTIME, spec);
}

int main(int argc, char** argv) {
    struct timespec spec;
    int i;
    for (i = 0; i < 100000000; i += 1) {
        janet_gettime(&spec);
    }
    printf("%ld\n", spec.tv_sec);
    return 0;
}

bench2.c:

#include <time.h>
#include <stdio.h>

#include <mach/clock.h>
#include <mach/mach.h>
int janet_gettime(struct timespec *spec) {
    clock_serv_t cclock;
    mach_timespec_t mts;
    host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock);
    clock_get_time(cclock, &mts);
    mach_port_deallocate(mach_task_self(), cclock);
    spec->tv_sec = mts.tv_sec;
    spec->tv_nsec = mts.tv_nsec;
    return 0;
}

int main(int argc, char** argv) {
    struct timespec spec;
    int i;
    for (i = 0; i < 1000000; i += 1) {
        janet_gettime(&spec);
    }
    printf("%ld\n", spec.tv_sec);
    return 0;
}
cell@indium$ gcc -Wall -o bench1 bench1.c && time ./bench1 
1647914319

real	0m1.385s
user	0m1.328s
sys	0m0.002s

cell@indium$ gcc -Wall -o bench2 bench2.c && time ./bench2 
1647914324

real	0m1.536s
user	0m0.279s
sys	0m1.206s

So I can call clock_gettime 100 million times in about the same runtime as calling the shim 1 million times.

(I also tried -O2 which didn't make any difference).

@cellularmitosis
Copy link
Contributor Author

Looks like the tests failed on windows:

Starting suite 9...
✘ "localname should match peername: msg=\"127.0.0.1 57922 127.0.0.1 8000\", buf=@\"57922\"": false
✘ "localname should match peername: msg=\"8000\", buf=@\"57923\"": false
✘ "localname should match peername: msg=\"57923\", buf=@\"8000\"": false
✘ "localname should match peername: msg=\"8000\", buf=@\"57924\"": false
✘ "localname should match peername: msg=\"57924\", buf=@\"8000\"": false

Hmm, that doesn't feel related to the PR. Is this just a CI burp?

@bakpakin
Copy link
Member

Yeah, that is just some unreliable networking for windows - an open bug that I've been unsuccessful at diagnosing.

@bakpakin bakpakin merged commit 9d0da74 into janet-lang:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants