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

fix: Support uint64 in crash reports #2631

Merged
merged 3 commits into from
Jan 20, 2023
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 19, 2023

📜 Description

Add support for uint64 when decoding crash reports.

💡 Motivation and Context

Applied from kstenerud/KSCrash#375. Thanks, @GLinnik21, for the improvement.

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Add support for uint64 when decoding crash reports.
@github-actions
Copy link

github-actions bot commented Jan 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.94 ms 1246.79 ms 24.85 ms
Size 20.76 KiB 419.67 KiB 398.91 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
56ec5d0 1194.39 ms 1236.94 ms 42.55 ms
3f1be0f 1208.12 ms 1225.72 ms 17.60 ms
c6504da 1232.06 ms 1243.28 ms 11.22 ms
feba2be 1246.67 ms 1254.64 ms 7.97 ms
56ec5d0 1236.65 ms 1261.90 ms 25.25 ms
cb2eefe 1222.04 ms 1243.04 ms 21.00 ms
dc0db9e 1222.10 ms 1240.90 ms 18.80 ms
b2f82fa 1237.78 ms 1256.02 ms 18.24 ms
302ee8b 1194.02 ms 1223.34 ms 29.32 ms
7c5d161 1224.38 ms 1249.66 ms 25.28 ms

App size

Revision Plain With Sentry Diff
56ec5d0 20.76 KiB 414.44 KiB 393.68 KiB
3f1be0f 20.76 KiB 414.44 KiB 393.69 KiB
c6504da 20.76 KiB 414.44 KiB 393.69 KiB
feba2be 20.76 KiB 414.45 KiB 393.69 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.69 KiB
cb2eefe 20.76 KiB 419.62 KiB 398.86 KiB
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
302ee8b 20.76 KiB 419.62 KiB 398.87 KiB
7c5d161 20.76 KiB 414.44 KiB 393.68 KiB

Previous results on branch: fix/crash-report-uint64

Startup times

Revision Plain With Sentry Diff
209233d 1218.23 ms 1239.29 ms 21.06 ms
c784236 1227.45 ms 1255.24 ms 27.79 ms

App size

Revision Plain With Sentry Diff
209233d 20.76 KiB 419.67 KiB 398.91 KiB
c784236 20.76 KiB 419.67 KiB 398.91 KiB

@GLinnik21
Copy link

You're welcome)

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

LGTM, just some questions and suggestions but no blockers!

Sources/SentryCrash/Recording/Tools/SentryCrashJSONCodec.c Outdated Show resolved Hide resolved
}

- (void)testSerializeDeserializeArrayFloat2
{
NSError *error = (NSError *)self;
NSString *expected = @"[-2e-15]";
id original = [NSArray arrayWithObjects:[NSNumber numberWithFloat:-2e-15f], nil];
float value = -2e-15f;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a special value in any way? If we're trying to target the smallest representable magnitude, we could try using something like -2 * powf(10, FLT_MIN_10_EXP) or FLT16_MIN_10_EXP.

Copy link
Member Author

@philipphofmann philipphofmann Jan 20, 2023

Choose a reason for hiding this comment

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

Good point, but it was already like that. I don't want to change this in this PR, I only simplified the code a bit.

@armcknight
Copy link
Member

In case you're wondering about the force push from me, I meant to send a fixup commit to one of your other PRs but hadn't paid attention to a failed branch switch locally, so instead committed it to this branch 🙃 All is as it was before I started meddling now.

@philipphofmann philipphofmann merged commit 15b8c61 into main Jan 20, 2023
@philipphofmann philipphofmann deleted the fix/crash-report-uint64 branch January 20, 2023 08:20
philipphofmann added a commit that referenced this pull request Jan 23, 2023
Follow up on #2631 to use uint64 instead of double for encoding
and decoding crash reports.
philipphofmann added a commit that referenced this pull request Jan 23, 2023
Add support for uint64 when decoding crash reports.
philipphofmann added a commit that referenced this pull request Jan 24, 2023
Follow up on #2631 to use uint64 instead of double for encoding
and decoding crash reports.
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.

3 participants