ARM-software / trappy

This repository has moved to https://gitlab.arm.com/tooling/trappy
Apache License 2.0
60 stars 39 forks source link

Remove raw trace output #253

Closed credp closed 7 years ago

credp commented 7 years ago

In the quest to speed up trappy parsing, I noticed that we can use the -r option for trace-cmd report so that we can use per-event raw parsing instead of generating a whole new text file.

This set of patches slightly alters the initialisation of parse objects so that we can have the set of events to use ready before we create the file, and then scans them to find which need raw formatting or not.

Then I use that to generate a single file. The patches look a little scrappy, but it works for me using the trappy example trace.

credp commented 7 years ago

I did some updates in order to pass the tests. I found a bug in systrace parsing, and the rest of the changes are in the tests themselves as they are not testing the correct things when you have raw and formatted events mixed in together rather than in separate files.

sinkap commented 7 years ago

From offline (er.. this is more offline :) ) discussion.

This is really awesome. Good riddance .raw.txt !

credp commented 7 years ago

I pushed an update with the stuff we talked about earlier. It passes all the tests for me, fingers crossed.

This is what you get if you run against an old trace.raw.txt file.

RuntimeError: You appear to be parsing both raw and formatted trace files. TRAPpy now uses a unified format. If you have the ./example_results/trace.dat file, remove the .txt files and try again. If not, you can manually move lines with the following events from ./example_results/trace.raw.txt to ./example_results/trace.txt : "sched_wakeup_new" "sched_switch" "sched_wakeup"

credp commented 7 years ago

OK, last 2 comments addressed. New branch pushed.

joelagnel commented 7 years ago

LGTM. Makes the whole report generation less confusing. @credp , could you please rebase on top of https://github.com/ARM-software/trappy/pull/247 since that was pushed first and if @bjackman is back with his last fix for that, then merging that is imminent :)

joelagnel commented 7 years ago

@credp I love these patches the more I look at them. Also hopefully rebasing them ontop of the caching ones isn't much trouble for you, but let me know if I can help with that! Also I figure at some point we should add time-based tests as well to ensure speed up related fixes don't regress!

credp commented 7 years ago

I was looking at the issue list earlier, and I think this should fix issue #210.

derkling commented 7 years ago

@joelagnel performance regression tests would be awesome... but I doubt it's something easy to implement, especially considering the different platform we use for development.

Perhaps we can just point people to a "golden trace" and ask every contributor to cross-check using this trace that there are not sensible time regressions with-vs-without the patches of a pull request.

Do you have any other idea specifically in mind? In any case we should move this discussion into a new issue: can you do that?

derkling commented 7 years ago

@credp I agree, this PR fixes the fundamental issue described in #210.

joelagnel commented 7 years ago

@derkling yes you are right its hard to implement. I will create a new issue and we can chat there.

sinkap commented 7 years ago

Merged! :+1: