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

Skip the cache of DWARFInfo and CU. #540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ThinkerYzu
Copy link

@ThinkerYzu ThinkerYzu commented Feb 12, 2024

Add DWARFInfo.skip_cache() and DWARFInfo.enable_cache() to give users the ability of controlling cache.

For the case of parsing the DWARF of a large binary, we may want to skip the cache to release the memory ASAP, avoiding extra CPU cycles on maintaining a cache.

One of my use cases is to extract types, functions, and call sites from the DWARF of a Linux kernel image. With caches, it takes about 573 seconds to go through all DIEs. Skipping caches reduces time to 448 seconds. It is about 27% faster. When going through every DIEs sequentially, cache doesn't help use at all.

Add DWARFInfo.skip_cache() and DWARFInfo.enable_cache() to give users
the ability of controlling cache.

For the case of parsing the DWARF of a large binary, we may want to
skip the cache to release the memory ASAP, avoiding extra CPU cycles
on maintaining a cache.

One of my use cases is to extract types, functions, and call sites
from the DWARF of a Linux kernel image. With caches, it takes about
573 seconds to go through all DIEs. Skipping caches reduces time to
448 seconds. It is about 27% faster. When going through every DIEs
sequentially, cache doesn't help use at all.
Copy link
Owner

@eliben eliben left a comment

Choose a reason for hiding this comment

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

This feels like a very targeted patch for a specific problem. If I were to accept it, I'd like some more systematic thinking about the approach. The switch should be positive (enable cache, not skip cache), there should be much more comments and there should be tests.

@ThinkerYzu
Copy link
Author

The switch should be positive (enable cache, not skip cache), there should be much more comments and there should be tests.

Do you mean that caches should be disabled by default?

@eliben
Copy link
Owner

eliben commented Feb 17, 2024

The switch should be positive (enable cache, not skip cache), there should be much more comments and there should be tests.

Do you mean that caches should be disabled by default?

No.

It can be something like "enable cache" which is true by default but can be set to false if needed

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.

None yet

3 participants