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

perf: cache debug images #1965

Closed
wants to merge 3 commits into from
Closed

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Jul 13, 2022

📜 Description

We're now caching the list of debug images, and invalidate the cache with the _dyld_register_func_for_add_image and _dyld_register_func_for_remove_image hooks.

#skip-changelog

💡 Motivation and Context

Closes #1096

💚 How did you test it?

Unit test

📝 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

@kevinrenskers
Copy link
Contributor Author

I feel kinda lost with this ticket, as I know very very little about C code, static/dynamic libraries, or even how to see these in the Sentry UI. And how would we test this? Do we load a dynamic library in the unit tests? How?

I'm sorry but this is a bit above my knowledge level, and I am going to need some help.

@brustolin
Copy link
Contributor

For tests you create a mock SentryCrashImage provider with test data. and call the same methods _dyld_register_func_for_add_image and _dyld_register_func_for_remove_image would call to see whether SentryDebugimageProvider recorded the changes.

@kevinrenskers
Copy link
Contributor Author

For tests you create a mock SentryCrashImage provider with test data. and call the same methods _dyld_register_func_for_add_image and _dyld_register_func_for_remove_image would call to see whether SentryDebugimageProvider recorded the changes.

Right, of course. But I mean how do we make sure those 2 hooks are actually getting called, and clearing the cache? Mocking it and calling it ourself doesn't test that.

* master:
  ref: Fix linter error (#1981)
  fix: read free_memory when the event is captured, not only at SDK startup (#1962)
  fix: Remove Sentry keys from cached HTTP request headers (#1975)
  release: 7.21.0
  ci: Don't run benchmarks on release (#1971)
  Don't track OOMs for simulators (#1970)
  feat: Automatic nest new spans with the ui life cycle function (#1959)
  docs: update some docs/comments to read a little better (#1966)
  ci: benchmarking updates (#1926)
  feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910)
@philipphofmann
Copy link
Member

For tests you create a mock SentryCrashImage provider with test data. and call the same methods _dyld_register_func_for_add_image and _dyld_register_func_for_remove_image would call to see whether SentryDebugimageProvider recorded the changes.

Right, of course. But I mean how do we make sure those 2 hooks are actually getting called, and clearing the cache? Mocking it and calling it ourself doesn't test that.

I think it is fine to test once manually if these hooks get actually called with manually loading a lib during the runtime, maybe this works. Then we can keep the mocks for the rest. Ideally, we could load and unload libs during the tests, we could use dlopen and dlclose for this. Maybe this example helps.

Comment on lines +43 to +45
_dyld_register_func_for_add_image(clearCachedDebugImages);
_dyld_register_func_for_remove_image(clearCachedDebugImages);

Copy link
Member

Choose a reason for hiding this comment

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

In some cases your callback function may be called on a different thread (namely the thread that is loading the library), see here: https://twitter.com/gparker/status/1266572192126386176?s=20&t=jXOtWHAPuZ8oUrlUj788jQ

In this scenario the callback functions are accessing cachedDebugImages in a non-thread safe way, so you would need to use a lock to guard all reads/writes to cachedDebugImages.

@brustolin
Copy link
Contributor

What do we need to finish this?

@kevinrenskers
Copy link
Contributor Author

Philip said that basically he was going to take over this PR because it was going to be part of bigger changes. I'm not really sure what his idea was, and if we can still continue with this and get it merged (after addressing the feedback).

@brustolin
Copy link
Contributor

Oh, ok.
I remember, we have a bigger problem regarding this images being fetch during a crash.

@github-actions
Copy link

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@philipphofmann
Copy link
Member

I think we should revisit this PR after fixing #1892, as both issues will touch similar code, and #1892 has a higher priority. So basically this PR is blocked by #1892.

@philipphofmann philipphofmann deleted the perf/1096-cache-debug-images branch March 4, 2024 07:47
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.

perf: Caching loaded symbols list
4 participants