search

ARM-software / trappy

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

systrace parsing new edition #249

Closed joelagnel closed 7 years ago

joelagnel commented 7 years ago

Hi @derkling , I went ahead and re-wrote #243. This is very fresh so I am just posting it early for any comments. This approach is more cleaner and also doesn't have issues with #243. It also doesn't require dropping custom strings written by tracing_mark_write that systrace doesn't recognize . I didn't add your test case but I ran some scripts to see it working and existing nosetests are passing. I would appreciate if you can integrate your test case from #243 to work with this PR and feel free to repost this PR with that. We can also close that old PR and work with this one. I borrowed your regex from the systrace format patch and given you credit in the relevant patch, thanks. CC @bjackman

bjackman commented 7 years ago

If we add a specific parser class for tracing_mark_write, does that break

bjackman commented 7 years ago

I don't grok the existing parsing code but this seems good to my slightly inexpert eyes. My only concern (aside from the little comments on the tracer attribute) is this: If we add a parser class for "tracing_mark_write" do we break injecting custom events from userspace (i.e. not systrace)? For example in LISA we use devlib to inject "cpu_frequency_devlib" events, which are really "tracing_mark_write" events containing the string "cpu_frequency_devlib:". We pass "cpu_frequency_devlib" in the events parameter to FTrace which results in a dynamic parser class being created with that as its unique word. Would that be broken if there was already a parser class with "tracing_mark_write" as its unique word?

joelagnel commented 7 years ago

Brendan you closed this PR, hope that wasn't intentional? ;)

You are right this will break that. It could be easily fixed though by making sure tracing mark write class is used only when nothing else matched. I will work on fixing that. I will also add better commit message for explaining the parsing.

bjackman commented 7 years ago

Brendan you closed this PR, hope that wasn't intentional? ;)

Yep, accident, sorry!

sinkap commented 7 years ago

Okay. I am merging this one. Thanks everyone!

joelagnel commented 7 years ago

Merged right?