Skip to content
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

Fix import of trace event files where B/E events' args don't match #321

Merged
merged 6 commits into from
Oct 25, 2020

Conversation

jlfwong
Copy link
Owner

@jlfwong jlfwong commented Oct 25, 2020

In #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

image

warning: ts=2: Request to end "gamma" when "beta" was on the top of the stack. Doing nothing instead.

chrome://tracing

image

@coveralls
Copy link

coveralls commented Oct 25, 2020

Coverage Status

Coverage increased (+0.3%) to 47.838% when pulling b2579aa on jlfwong/trace-event-mismatch-leniency into aee2dfd on master.

@jlfwong jlfwong force-pushed the jlfwong/trace-event-mismatch-leniency branch from 27e08b2 to f0d1de5 Compare October 25, 2020 04:40
@jlfwong jlfwong merged commit de3ab89 into master Oct 25, 2020
@jlfwong jlfwong deleted the jlfwong/trace-event-mismatch-leniency branch October 25, 2020 04:58
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
![image](https://user-images.githubusercontent.com/150329/97098205-7365dd00-1637-11eb-9869-4e81ebebcee1.png)
```
warning: ts=2: Request to end "gamma" when "beta" was on the top of the stack. Doing nothing instead.
```
### chrome://tracing
![image](https://user-images.githubusercontent.com/150329/97098215-87114380-1637-11eb-909c-b2e70c7291a4.png)
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.

2 participants