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

[PLAT-7804] Improve crash report writing performance with buffered output #1281

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

nickdowell
Copy link
Contributor

@nickdowell nickdowell commented Jan 12, 2022

Goal

Improve the performance of writing crash reports, to reduce the likelihood of apps being terminated (by watchdog or user) before finishing.

Design

Use write buffers to reduce the number of calls to write().

⚠️ Buffers are allocated on the stack since they cannot be malloc()'d at run-time due to the constrained crash-time environment. Thoughts on the safety of this are welcome - perhaps static allocation should be used instead at the expense of thread safety (which is not something we support in the crash handler.)

Changeset

  • Implements write buffering in BSG_KSFile.c.
  • Modifies BSG_KSCrashReport.c to use buffered output.
  • Modifies BSG_KSCrashState.m to use buffered output.

Testing

Tested by updating unit tests BSG_KSCrashReportTests and KSCrashState_Tests.

Also verified by manually inspecting crash report files and running full E2E suite on CI. Bugs in file writing would manifest as unreported crashes.

The performance unit test case shows improvements of 40-60% depending on the device.

Profiling shows that around 95% of bsg_kscrashreport_writeStandardReport()'s time is now spent in bsg_symbolicate().

@github-actions
Copy link

Infer: No issues found 🎉

OCLint: No issues found 🎉

Bugsnag.framework binary size increased by 1,008 bytes from 1,298,728 to 1,299,736

Generated by 🚫 Danger

Copy link
Contributor

@kstenerud kstenerud left a comment

Choose a reason for hiding this comment

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

I think we need some isolated tests of BSG_KSFile to check all edge cases...

Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSFile.c Outdated Show resolved Hide resolved
@kstenerud
Copy link
Contributor

kstenerud commented Jan 13, 2022

Allocations don't have to all happen on the stack. In a crashed context there will only be one thread that can use it at a time, so a globally pre-allocated buffer will suffice. On a recrash, you just reinit the buffering mechanism since the previous crash handler has been stopped in its tracks.

@nickdowell nickdowell force-pushed the nickdowell/kscrash-write-buffer branch from 0949b2d to 099c5a7 Compare January 13, 2022 11:04
@nickdowell
Copy link
Contributor Author

I think we need some isolated tests of BSG_KSFile to check all edge cases...

Added a unit test that covers all the non-error code.

@nickdowell nickdowell requested a review from kstenerud January 13, 2022 11:05
@nickdowell nickdowell merged commit d9610e6 into next Jan 18, 2022
@nickdowell nickdowell deleted the nickdowell/kscrash-write-buffer branch January 18, 2022 16:33
@nickdowell nickdowell mentioned this pull request Jan 19, 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