-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
ref: Add nanosecondsToTimeInterval #3483
Conversation
SentryTime has timeIntervalToNanoseconds but not nanosecondsToTimeInterval. This PR adds nanosecondsToTimeInterval.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8f397a7 | 1230.10 ms | 1253.88 ms | 23.77 ms |
371db89 | 1226.40 ms | 1251.54 ms | 25.14 ms |
94b89eb | 1236.08 ms | 1264.58 ms | 28.50 ms |
49819af | 1263.92 ms | 1275.66 ms | 11.74 ms |
736495a | 1245.16 ms | 1254.42 ms | 9.26 ms |
dacf894 | 1223.96 ms | 1250.41 ms | 26.45 ms |
e998fd0 | 1254.84 ms | 1265.60 ms | 10.76 ms |
734d507 | 1201.78 ms | 1213.96 ms | 12.18 ms |
14a1101 | 1242.71 ms | 1272.02 ms | 29.31 ms |
dbc67d2 | 1239.49 ms | 1248.88 ms | 9.39 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8f397a7 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
371db89 | 20.76 KiB | 427.31 KiB | 406.55 KiB |
94b89eb | 20.76 KiB | 399.20 KiB | 378.43 KiB |
49819af | 20.76 KiB | 427.31 KiB | 406.55 KiB |
736495a | 20.76 KiB | 435.22 KiB | 414.46 KiB |
dacf894 | 20.76 KiB | 426.34 KiB | 405.58 KiB |
e998fd0 | 21.58 KiB | 414.59 KiB | 393.01 KiB |
734d507 | 22.85 KiB | 412.66 KiB | 389.81 KiB |
14a1101 | 20.76 KiB | 436.25 KiB | 415.49 KiB |
dbc67d2 | 20.76 KiB | 427.74 KiB | 406.98 KiB |
Previous results on branch: ref/add-nanosecondsToTimeInterval
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0f43c5b | 1216.85 ms | 1246.72 ms | 29.87 ms |
ff1612e | 1224.90 ms | 1246.29 ms | 21.39 ms |
d84c1d6 | 1262.39 ms | 1281.00 ms | 18.61 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0f43c5b | 21.58 KiB | 414.92 KiB | 393.34 KiB |
ff1612e | 21.58 KiB | 414.95 KiB | 393.37 KiB |
d84c1d6 | 21.58 KiB | 414.92 KiB | 393.34 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with some suggestions
return (uint64_t)(seconds * NSEC_PER_SEC); | ||
} | ||
|
||
double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double | |
NSTimeInterval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use NSTimeInterval
because of SENTRY_EXTERN_C_BEGIN
. I think it's fine to keep the double. I'm not that familiar with the C and C++ interop. What would we need to do to remove the SENTRY_EXTERN_C_BEGIN
, @armcknight?
@@ -10,9 +10,15 @@ | |||
timeIntervalToNanoseconds(double seconds) | |||
{ | |||
NSCAssert(seconds >= 0, @"Seconds must be a positive value"); | |||
NSCAssert(seconds <= UINT64_MAX / 1e9, | |||
NSCAssert(seconds <= (double)UINT64_MAX / (double)NSEC_PER_SEC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSCAssert(seconds <= (double)UINT64_MAX / (double)NSEC_PER_SEC, | |
NSCAssert(seconds <= (NSTimeInterval)UINT64_MAX / (NSTimeInterval)NSEC_PER_SEC, |
Co-authored-by: Andrew McKnight <andrew.mcknight@sentry.io>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3483 +/- ##
=============================================
+ Coverage 89.033% 89.099% +0.065%
=============================================
Files 525 526 +1
Lines 56710 56934 +224
Branches 20145 20484 +339
=============================================
+ Hits 50491 50728 +237
+ Misses 5300 5174 -126
- Partials 919 1032 +113
... and 96 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
SentryTime has timeIntervalToNanoseconds but not nanosecondsToTimeInterval. This PR adds nanosecondsToTimeInterval.
Furthermore, added some tests for timeIntervalToNanoseconds and fixed the max value check. This method is required for frame delay #3481.
#skip-changelog