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

use a stable sort for trace events #2415

Merged
merged 3 commits into from
Jun 22, 2017
Merged

use a stable sort for trace events #2415

merged 3 commits into from
Jun 22, 2017

Conversation

brendankenny
Copy link
Member

fixes #2333. Ensures that nested trace events stay nested.

I'm loath to use the Devtools array extensions, but the easiest way to bring in a stable sort is to use the one we already pull in with devtools-frontend, and since it writes it directly onto the Array prototype, the easiest way to use it is to just call it on the traceEvents array. Better than bringing in a new module, at least :S

For testing, I haven't yet been able to produce a trace that when sorted with the native v8 [].sort will put events from the same process and thread out of order. In #2333 @wwwillchen notes it may also rely on the poor resolution of the android timer, so I'll try grabbing a trace from a mobile device to see if that gets me what we'll need.

@patrickhulce
Copy link
Collaborator

@brendankenny it's not clear how this fixes #2333. Do you have a repro trace we can test? Since these are filtered views on the full trace events this doesn't explain how the exported trace would have its events out of order and fixing it here actually doesn't give us anything since we don't care to look at nested events from these arrays, right?

@brendankenny
Copy link
Member Author

it's not clear how this fixes #2333

errrr, very good point. I'll get on that.

Do you have a repro trace we can test?

I write the PR description for a reason, you know :P

@patrickhulce
Copy link
Collaborator

I write the PR description for a reason, you know :P

I meant that fixes whatever you were trying to fix instead :P

@paulirish
Copy link
Member

@brendankenny AFAIK this PR is waiting on you grabbing a trace from mobile?

@brendankenny
Copy link
Member Author

AFAIK this PR is waiting on you grabbing a trace from mobile?

getting a trace from mobile that sorted incorrectly wasn't working for me, and the trace invariants seem to be more than just that they're nested correctly ts-wise, making them difficult to verify.

Instead the latest commit adds tests that simply ensure that speedline doesn't alter the order of the trace it gets and that TraceOfTab.processEvents was definitely stably sorted. Verified that without the stable sort changes that both tests fail with these (existing) test traces.

@brendankenny
Copy link
Member Author

Once we have GAR, we should maybe have a set of integration tests in between our unit tests and the full smoke tests (which test with a launched chrome).

These tests check that the two places we currently sort trace events do so correctly, but it would be great to assert that the full default config running over a set of artifacts leaves the final trace correctly sorted, no matter the audits we land in the future.

@paulirish paulirish merged commit 7d12091 into master Jun 22, 2017
@paulirish paulirish deleted the stablesort branch June 22, 2017 00:22
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.

Errors in recorded traces
3 participants