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 for Chrome 69, support leading idle time before first call #160

Merged
merged 4 commits into from Sep 10, 2018

Conversation

2 participants
@jlfwong
Owner

jlfwong commented Sep 9, 2018

This PR fixes #159, and also fixes various small things about how profiles were imported for previous versions of Chrome & for Firefox.

The Chrome 69 format splits profiles across several Trace Event Format events. There are two relevant events: "Profile" and "ProfileChunk". At first read through a profile, it seems like profiles are incorrectly terminated, but it seems like the cause of that is that, for whatever reason, events in the event log are not always sorted in chronological order. If sorted chronologically, then the event sequence can be parsed sensibly.

In the process of looking at this information, I also discovered that speedscope's chrome importer was incorrectly interpreting the value of the first element in timeDeltas array. It's intended to be the elapsed time since the start of the profile, not the time between the first pair of samples. This changes the weight attributed to the first sample.

@coveralls

This comment has been minimized.

coveralls commented Sep 9, 2018

Coverage Status

Coverage decreased (-0.02%) to 42.145% when pulling 256b1af on jlfwong/chrome-69 into a1f9755 on master.

@jlfwong jlfwong merged commit b910a20 into master Sep 10, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 42.145%
Details

@jlfwong jlfwong deleted the jlfwong/chrome-69 branch Sep 10, 2018

jlfwong added a commit that referenced this pull request Sep 13, 2018

Fix import from Chrome < 69 when there are multiple profiles
It seems like #160 accidentally broken import of profiles in some circumstances from Chrome < 69. Before #160, we always took the first profile in the list *but* the profiles were not sorted chronologically. After #160 but before this PR, we were taking the chronologically first.

After this PR, we always take the chronologically last `CpuProfile` event in the trace.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment