Skip to content

reporter: respect existing elements in dictionary #538

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

Closed
wants to merge 1 commit into from

Conversation

florianl
Copy link
Contributor

@florianl florianl commented Jun 17, 2025

The helper elements, like stringMap and FuncMap, in setProfile() assumed, that they are the only ones and always the first that write to the protocol global dictionaries. This is not correct and so while the content of these helper elements were appended to the according dictionaries, the references were not correct (as they always started at 0).

Fixes #534

Thanks @fandreuz for reporting this issue.

@florianl florianl requested review from a team as code owners June 17, 2025 13:28
}

// When ranging over stringMap, the order will be according to the
// hash value of the key. To get the correct order for profile.StringTable,
// put the values in stringMap, in the correct array order.
stringTable := make([]string, len(stringMap))
tmpStringTable := make([]string, len(stringMap)+int(strDicOffset))
Copy link
Contributor Author

@florianl florianl Jun 17, 2025

Choose a reason for hiding this comment

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

Yes - this can become a massive waste of memory, as strDicOffset of elements are just empty elements ...
dic.StringTable() is missing a functionality like AppendEmpty(), as EnsureCapacity() is not sufficient to grow the underlying data structure.

The helper elements, like stringMap and FuncMap, in setProfile() assumed, that they are the only and always the first that write to protocol global dictionaries. This is not correct and so while the content of these helper elements were appended to the according dictionaries, the references were not correct (as they always started at 0).

Fixes #534

Thanks @fandreuz for reporting this issue.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

st := profile.SampleType().AppendEmpty()
switch origin {
case support.TraceOriginSampling:
st.SetTypeStrindex(getStringMapIndex(stringMap, "samples"))
st.SetUnitStrindex(getStringMapIndex(stringMap, "count"))
st.SetTypeStrindex(getStringMapIndex(stringMap, strDicOffset, "samples"))

Choose a reason for hiding this comment

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

In Async-Profiler we created a small abstraction to ease this process, and we fill the string table in order after all profiles have been processed. Maybe you could consider something similar? Passing around offsets seems a bit error prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider this more of a hot patch and let the options open for #515.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah right. Every table in ProfilesDictionary could benefit from such an approach. Not just functions and locations.

@christos68k
Copy link
Member

I fixed this in #550 by introducing a new type, libpf.OrderedSet

@florianl
Copy link
Contributor Author

Closing in favor of #550.

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.

setProfile() only works with one Profile per message
3 participants