-
Notifications
You must be signed in to change notification settings - Fork 321
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
Conversation
} | ||
|
||
// 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)) |
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I fixed this in #550 by introducing a new type, |
Closing in favor of #550. |
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.