Add some Profile compatibility routines #42482
Conversation
|
Interesting! One of the buildkite errors shows that occasionally the "stripped" data has the same magic sequence as the end-terminator in unstripped data. I can replicate this locally. Presumably it would be better to prevent this? |
IanButterworth
force-pushed
the
teh/profile_compat
branch
from
952912c
to
5ea8bd0
Compare
|
Buildkite failure is related, https://buildkite.com/julialang/julia-master/builds/4507#099ef975-964b-490d-a462-b3129151c01b |
|
As seen in the buildkite failures, The To fix it I narrowed the heuristic a bit further |
This makes it easier for external packages like FlameGraphs to continue to support Profile by hiding some of the details of the internal metadata format.
This reverts commit cc2ebd4.
IanButterworth
force-pushed
the
teh/profile_compat
branch
from
0a5dcf3
to
0e4e1a1
Compare
I'm a bit confused. This was not a heuristic before. Seems like this PR was just generating corrupt blocks of data? |
|
By heuristic, I mean the |
|
Yes, it isn't valid to call that on data that isn't formatted into blocks. |
|
Ok, so instead of making it work for both stripped and unstripped, would it be better to keep I'm also questioning whether it was even right to strip out the metadata in the first place. It was done to avoid breakage downstream, but consumers are using internals like |
|
yeah, we might want to add a versioning bit at the front, so we can continue to update this without causing breakage |
|
Yes, it's possible that the fundamental problem was stripping it out sounded nicer than it was. However, tests like these in FlameGraphs explicitly constructed raw Profile data and so end up being dependent on internals. Perhaps we need a utility in Profile to support construction of fake data so it can be better-insulated from the rest of the world? Two other observations:
|
As was discussed in #42482 after merge, there's some things that could be done better: - Reverts the is_block_end logic, given it should only be run on data that includes metadata, so can afford to be more efficient - Adds has_meta for identifying when profile data has metadata - Adds metadata checks to functions that expect metadata to be present - Changes Profile.fetch to by default include metadata. The reason for this is that it was previously made to strip metadata to avoid breaking existing downstream Profile data consumers, but it turns out that they use Profile internals like tree! that require the metadata to be present, so this ended up causing an unintended breakage anyway - Adds consts for the metadata field offsets, for consumers to use
As was discussed in JuliaLang#42482 after merge, there's some things that could be done better: - Reverts the is_block_end logic, given it should only be run on data that includes metadata, so can afford to be more efficient - Adds has_meta for identifying when profile data has metadata - Adds metadata checks to functions that expect metadata to be present - Changes Profile.fetch to by default include metadata. The reason for this is that it was previously made to strip metadata to avoid breaking existing downstream Profile data consumers, but it turns out that they use Profile internals like tree! that require the metadata to be present, so this ended up causing an unintended breakage anyway - Adds consts for the metadata field offsets, for consumers to use
As was discussed in JuliaLang#42482 after merge, there's some things that could be done better: - Reverts the is_block_end logic, given it should only be run on data that includes metadata, so can afford to be more efficient - Adds has_meta for identifying when profile data has metadata - Adds metadata checks to functions that expect metadata to be present - Changes Profile.fetch to by default include metadata. The reason for this is that it was previously made to strip metadata to avoid breaking existing downstream Profile data consumers, but it turns out that they use Profile internals like tree! that require the metadata to be present, so this ended up causing an unintended breakage anyway - Adds consts for the metadata field offsets, for consumers to use
This makes it easier for external packages like FlameGraphs to continue
to support Profile by hiding some of the details of the internal
metadata format.
It also fixes a couple of typos in docstrings and comments.