Fix import of trace event files where B/E events' args don't match #321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jlfwong
force-pushed
the
jlfwong/trace-event-mismatch-leniency
branch
from
27e08b2
to
f0d1de5
Compare
jackerghan
pushed a commit
to jackerghan/speedscope
that referenced
this pull request
Jul 28, 2023
…lfwong#321) In jlfwong#273, I changed `CallTreeProfileBuilder.leaveFrame` to fail hard when you request to leave a frame different from the one at the top of the stack. It turns out we were intentionally doing this for trace event imports, because `args` are part of the frame key, and we want to allow profiles to be imported where the `"B"` and `"E"` events have differing `args` field. This PR fixes the import code to permissively allow the `"args"` field to not match between the `"B"` and `"E"` fields. **A note on intentional differences between speedscope and chrome://tracing** `chrome://tracing` will close whichever frame is at the top when it gets an `"E"` event, regardless of whether the name or the args match. speedscope will ignore the event entirely if the `"name"` field doesn't match, but will warn but still close the frame if the `"name"`s match but the `"args"` don't. ``` [ {"pid": 0, "tid": 0, "ph": "B", "name": "alpha", "ts": 0}, {"pid": 0, "tid": 0, "ph": "B", "name": "beta", "ts": 1}, {"pid": 0, "tid": 0, "ph": "E", "name": "gamma", "ts": 2}, {"pid": 0, "tid": 0, "ph": "E", "name": "beta", "ts": 9}, {"pid": 0, "tid": 0, "ph": "E", "name": "alpha", "ts": 10} ] ``` ### speedscope  ``` warning: ts=2: Request to end "gamma" when "beta" was on the top of the stack. Doing nothing instead. ``` ### chrome://tracing 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #273, I changed
CallTreeProfileBuilder.leaveFrameto fail hard when you request to leave a frame different from the one at the top of the stack. It turns out we were intentionally doing this for trace event imports, becauseargsare part of the frame key, and we want to allow profiles to be imported where the"B"and"E"events have differingargsfield.This PR fixes the import code to permissively allow the
"args"field to not match between the"B"and"E"fields.A note on intentional differences between speedscope and chrome://tracing
chrome://tracingwill close whichever frame is at the top when it gets an"E"event, regardless of whether the name or the args match. speedscope will ignore the event entirely if the"name"field doesn't match, but will warn but still close the frame if the"name"s match but the"args"don't.speedscope
chrome://tracing