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

Memory Termination Support #474

Closed
naftaly opened this issue May 9, 2024 · 7 comments · Fixed by #476
Closed

Memory Termination Support #474

naftaly opened this issue May 9, 2024 · 7 comments · Fixed by #476

Comments

@naftaly
Copy link
Contributor

naftaly commented May 9, 2024

Is there any interest in integrating memory termination support directly into KSCrash, or is this something we want to leave up to those who integrate KSCrash?

Here's what I've done for Sentry, I'd love to bring this into KSCrash so a lot more teams get to process OOMs more efficiently.

@GLinnik21
Copy link
Collaborator

Hey, thanks so much for your commitment to improving KSCrash! Integrating memory termination support directly into the library sounds like a fantastic idea. I know a lot of teams have been using the algorithm from that Facebook article to handle OOMs on their own, so having this built into KSCrash could be super beneficial for everyone.

@bamx23 what do you think?

@bamx23
Copy link
Collaborator

bamx23 commented May 9, 2024

Wow, thanks! Yes, definitely worth bringing to KSCrash. OOMs are one of the most tricky types of crashes so any work towards making them easier to detect is a great work!

@naftaly feel free to create a PR to v2.0 branch and we can discuss the implementation there. Or if you want we can start with overall concept of the implementation here in this issue.

@naftaly
Copy link
Contributor Author

naftaly commented May 10, 2024

@bamx23 There are some great comments here and here in my Crashlytics PR that can help give an idea of how it works.

I'll put a diff in the next days.

@naftaly
Copy link
Contributor Author

naftaly commented May 11, 2024

@bamx23 @GLinnik21 I've put some work into this over the lats few days. Have a look at this branch in my fork and when you have a chance and tell me what you think. It's not totally done but the idea is there.

@naftaly
Copy link
Contributor Author

naftaly commented May 11, 2024

Not OOM related...

One thing that could make the system even more robust is "install folders". Basically, for each install there is a place on disk where things go which is different from the previous session (using GUIDs or something).

This allows for a few things:

  • No need to rush at startup to store previous data because it'll be overwritten by new data.
  • Failures to report don't matter since you can always process later.
  • ...

@GLinnik21
Copy link
Collaborator

@bamx23 @GLinnik21 I've put some work into this over the lats few days. Have a look at this branch in my fork and when you have a chance and tell me what you think. It's not totally done but the idea is there.

Hey, awesome work on this! I've gone through the code on your branch and it's looking solid. I do have a couple of minor questions, but we can get into those details once you draft the PR. Could you go ahead and create the PR targeting the release-2.0 branch? We can continue the discussion there. Great job again!

@naftaly
Copy link
Contributor Author

naftaly commented May 12, 2024

PR Draft: #476

@GLinnik21 GLinnik21 linked a pull request May 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants