Skip to content

reporter: respect existing elements in dictionary#538

Closed
florianl wants to merge 1 commit intomainfrom
dict-offsets
Closed

reporter: respect existing elements in dictionary#538
florianl wants to merge 1 commit intomainfrom
dict-offsets

Conversation

@florianl
Copy link
Copy Markdown
Member

@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
// 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
Copy Markdown
Member 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>
case support.TraceOriginSampling:
st.SetTypeStrindex(getStringMapIndex(stringMap, "samples"))
st.SetUnitStrindex(getStringMapIndex(stringMap, "count"))
st.SetTypeStrindex(getStringMapIndex(stringMap, strDicOffset, "samples"))
Copy link
Copy Markdown

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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
Member

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

@florianl
Copy link
Copy Markdown
Member 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