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

reporter: extend lifetime for cached elements #260

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

florianl
Copy link
Contributor

@florianl florianl commented Dec 2, 2024

Similar to the reported issue #248 around lifetime of executables, the same issue happens for frames. So keep cached frame information alive.

Similar to the reported issue #248 around lifetime of executables, the same issue happens for frames.
So keep cached frame information alive.

Signed-off-by: Florian Lehner <[email protected]>
@florianl florianl requested review from a team as code owners December 2, 2024 09:11
@Gandem
Copy link
Contributor

Gandem commented Dec 2, 2024

👋 As discussed in #248, using GetAndRefreshKeys() for the frames outer cache solves the issue for UNREPORTED / UNRESOLVED frames, however, for long / forever running large executables, there is the potential of increasing the inner map (map[libpf.AddressOrLineno]sourceInfo) without limit.

I'm personally onboard with merging this first then working on the inner map size issue, just wanted to highlight the tradeoff that was raised in #248 (comment) !

@florianl
Copy link
Contributor Author

florianl commented Dec 4, 2024

Thanks for pointing this out. As mentioned, there is a risk of increasing the inner map (map[libpf.AddressOrLineno]sourceInfo) without limit. I do also agree, that limiting this inner map should be a dedicated issue/task.

@florianl florianl enabled auto-merge (squash) December 4, 2024 11:47
@florianl florianl merged commit 9eb4545 into main Dec 4, 2024
23 checks passed
@florianl florianl deleted the reporter-exec-keepalive branch December 4, 2024 18:02
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.

4 participants