Skip to content

reporter: replace inner regular map with LRU#451

Merged
florianl merged 2 commits intomainfrom
issue-250
Apr 29, 2025
Merged

reporter: replace inner regular map with LRU#451
florianl merged 2 commits intomainfrom
issue-250

Conversation

@florianl
Copy link
Copy Markdown
Member

In #248 the possibile unlimited growth of the inner map map[libpf.AddressOrLineno]sourceInfo of the frames cache, was identified as potential issue. Replace this regular map with a LRU to limit its size.

In #248 the possibile unlimited growth of the inner map map[libpf.AddressOrLineno]sourceInfo of the frames cache, was identified as potential issue.
Replace this regular map with a LRU to limit its size.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl florianl requested review from a team as code owners April 25, 2025 12:47
@rockdaboot
Copy link
Copy Markdown
Contributor

Should there be a regular call to PurgeExpired?

Comment thread reporter/base_reporter.go
frameMap := frameMapLock.RLock()
defer frameMapLock.RUnlock(&frameMap)
_, known = (*frameMap)[frameID.AddressOrLine()]
_, known = (*frameMap).GetAndRefresh(frameID.AddressOrLine(), pdata.FrameMapLifetime)
Copy link
Copy Markdown
Member

@christos68k christos68k Apr 25, 2025

Choose a reason for hiding this comment

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

@rockdaboot Not for this PR but it seems clumsy to have to specify the lifetime every time GetAndRefresh is called if SetLifetime has been previously called to set one (if the lifetime we're specifying isn't different from the one we've previously set). Assuming that this is the common case, maybe we should optimize for it:

GetAndRefresh can use the LRU lifetime and a new method (e.g. GetAndRefreshWithLifetime) can be added to support custom per-value lifetimes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be a breaking API change in go-freelru, but since we are 0.x, we can do this.
Created an issue: elastic/go-freelru#85

Comment thread reporter/base_reporter.go Outdated
Comment thread reporter/base_reporter.go Outdated
Comment thread reporter/base_reporter.go Outdated
Comment thread reporter/base_reporter.go Outdated
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
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.

3 participants