Skip to content

zend_hrtime: Use clock_gettime_nsec_np() for macOS if available #17089

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

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Dec 9, 2024

This is completely untested, because I do not have macOS available.


As per the Apple developer documentation:

Prefer to use the equivalent clock_gettime_nsec_np(CLOCK_UPTIME_RAW) in
nanoseconds.

and also

This API has the potential of being misused to access device signals to try
to identify the device or user, also known as fingerprinting. Regardless of
whether a user gives your app permission to track, fingerprinting is not
allowed. When you use this API in your app or third-party SDK (an SDK not
provided by Apple), declare your usage and the reason for using the API in
your app or third-party SDK’s PrivacyInfo.xcprivacy file.

see https://developer.apple.com/documentation/kernel/1462446-mach_absolute_time

As per the Apple developer documentation:

> Prefer to use the equivalent clock_gettime_nsec_np(CLOCK_UPTIME_RAW) in
> nanoseconds.

and also

> This API has the potential of being misused to access device signals to try
> to identify the device or user, also known as fingerprinting. Regardless of
> whether a user gives your app permission to track, fingerprinting is not
> allowed. When you use this API in your app or third-party SDK (an SDK not
> provided by Apple), declare your usage and the reason for using the API in
> your app or third-party SDK’s PrivacyInfo.xcprivacy file.

see https://developer.apple.com/documentation/kernel/1462446-mach_absolute_time
@TimWolla TimWolla force-pushed the macos-hrtime-gettime-nsec branch from c44dd4f to 2d4f3d2 Compare December 9, 2024 08:52
@TimWolla
Copy link
Member Author

TimWolla commented Dec 9, 2024

checking whether clock_gettime_nsec_np is declared... yes

In the CI output for macOS. So this is probably correct, unless I messed up the checks in the C code.

@TimWolla TimWolla marked this pull request as ready for review December 9, 2024 09:24
@TimWolla TimWolla requested review from petk and devnexen December 9, 2024 09:24
@devnexen
Copy link
Member

devnexen commented Dec 9, 2024

It is good, code-wise. I recall clock_gettime_nsec_np has a higher overhead but should be negligible in practice.

@edorian
Copy link
Member

edorian commented Dec 10, 2024

checking whether clock_gettime_nsec_np is declared... yes

In the CI output for macOS. So this is probably correct, unless I messed up the checks in the C code.

Compiled on an M3 Pro.

checking whether clock_gettime_nsec_np is declared... yes`
 #elif ZEND_HRTIME_PLATFORM_APPLE_GETTIME_NSEC
        printf("apple_gettime_nsec\n");
        return clock_gettime_nsec_np(CLOCK_UPTIME_RAW);

showed that it's hitting the newly added branch for me.


Rough measurements without the printf:

cat hrtime.php
<?php

$a = hrtime(true);
$b = hrtime(true);
echo $b - $a;
while true; do sapi/cli/php hrtime.php && echo; done
9750
750
917
791
875
875
3500
27708
833
1292
1209
1208

Copy link
Member

@petk petk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea about possible performance issues in this case but I think this is ok, yes. Thanks.

@TimWolla TimWolla merged commit 85f69a7 into php:master Dec 10, 2024
1 check passed
@TimWolla TimWolla deleted the macos-hrtime-gettime-nsec branch December 10, 2024 14:10
@jorgsowa
Copy link
Contributor

Can you please @TimWolla have a look at my PR? #17437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants