systrace parsing new edition #249

joelagnel · 2017-06-12T05:22:46Z

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 · 2017-06-12T10:35:14Z

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

bjackman · 2017-06-12T10:47:35Z

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 · 2017-06-12T14:21:47Z

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 · 2017-06-12T14:41:10Z

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

Yep, accident, sorry!

This is needed so that we're not dependent anymore on making sure data starts at something with '=' and will allow to parse custom systrace tracing_mark_write events which don't have an '=' in them. The new regex will also work for lines that do already have an '=' and doesn't change their behavior. The last line of the new regex in this patch works like this: r"(?P<timestamp>[0-9]+\.[0-9]+): (\w+:\s+)+(?P<data>.+)" We use skip over '<string>:<whitespace> ' and the next thing we find is considered the start of the data. This works well to find the start of a line for existing traces and is needed for systraces which don't have '='. Signed-off-by: Joel Fernandes <[email protected]>

In preparation of being able to parse systrace, split out data string processing into separate function so it can be overridden by sub classes. Signed-off-by: Joel Fernandes <[email protected]>

sinkap

Just some minor comments. Otherwise LGTM

sinkap · 2017-06-14T19:44:07Z

trappy/bare_trace.py

            trace_class.finalize_object()
+
+    def generate_data_dict(self):
+        return None


We should raise a NotImplemented exception if this is called. Or we can just remove this method no?

If we remove it, it will break code that calls it (since not all tracers are expected to implement it). Also not implementing it is not an exception but the normal fall back.

Then we should have this raise the exception and we should catch the exception and then implement the fallback. So that it can be more explicit

sinkap · 2017-06-14T20:00:34Z

trappy/tracing_mark_write.py

@@ -0,0 +1,33 @@
+#    Copyright 2017 ARM Limited, Google and contributors


Let's add a module docstring

sinkap · 2017-06-14T20:01:15Z

trappy/tracing_mark_write.py

+from trappy.base import Base
+from trappy.dynamic import register_ftrace_parser
+
+class TracingMarkWrite(Base):


Please add a docstring for the class. I know we have not been careful about doing this. I will probably need to backfill.

sinkap · 2017-06-14T20:03:01Z

trappy/systrace.py

 from trappy.ftrace import GenericFTrace
+import re

 class drop_before_trace(object):


we have been naming the classes as DropBeforeTrace. Also I am not sure what the name conveys. I will change that in a follow up PR.

sinkap · 2017-06-14T20:05:14Z

trappy/systrace.py

+
+        return data_dict
+
+SYSTRACE_EVENT = re.compile(


I would move this to the top

sinkap · 2017-06-14T20:07:08Z

trappy/base.py


    """
-    def __init__(self, parse_raw=False):
+    def __init__(self, parse_raw=False, is_backup=False):


Can you add to the documentation what is_backup actually means? Let's call it is_fallback as discussed.

Create tracing_mark_write as an event and treat it like such. It overrides data string processing since it cannot use 'key=value' parsing. Instead it relies on the tracer to provide one (such as systrace has custom trace format from userspace events). If tracer cannot provide one, then it just returns a default dictionary with key 'string' and value as the string in the event. Signed-off-by: Joel Fernandes <[email protected]>

Here we provide custom data string parsing for Android userspace events. Borrowed the regex and parsing logic from Patrick's patch. Co-developed-by: Patrick Bellasi <[email protected]> Signed-off-by: Joel Fernandes <[email protected]>

…_write Certain fake events can be injected using tracing_mark_write and should be classified into their own event classes than tracing_mark_write. One way to handle this is to use tracing_mark_write only as a backup. For this reason lets introduced the concept of backup event classes and classify TracingMarkWrite as that. This case normally doesn't happen because dynamic events are inserted into the beginning of the event list from what I've seen. Since tracing_mark_write is a built-in event, and not dynamic, its always at after and so is less preferred. However we should protect for the future case where there might be a built-in which happens to be after TrackingMarkWrite class in the list, so lets handle it now. Signed-off-by: Joel Fernandes <[email protected]>

Signed-off-by: Joel Fernandes <[email protected]>

sinkap · 2017-06-14T20:48:15Z

Okay. I am merging this one. Thanks everyone!

joelagnel · 2017-06-14T22:06:31Z

Merged right?

Update trappy and fix cpu_idle

bjackman closed this Jun 12, 2017

bjackman reopened this Jun 12, 2017

sinkap self-assigned this Jun 12, 2017

joelagnel changed the title ~~[RFC] systrace parsing new edition~~ systrace parsing new edition Jun 14, 2017

Joel Fernandes added 2 commits June 13, 2017 21:34

trappy: Split out parsing of trace string into separate function

11ce74e

In preparation of being able to parse systrace, split out data string processing into separate function so it can be overridden by sub classes. Signed-off-by: Joel Fernandes <[email protected]>

sinkap reviewed Jun 14, 2017

View reviewed changes

Joel Fernandes added 4 commits June 14, 2017 13:30

trappy/systrace: Provide custom data string parsing

c37168f

Here we provide custom data string parsing for Android userspace events. Borrowed the regex and parsing logic from Patrick's patch. Co-developed-by: Patrick Bellasi <[email protected]> Signed-off-by: Joel Fernandes <[email protected]>

trappy/systrace: Add test for parsing userspace events

15ea129

Signed-off-by: Joel Fernandes <[email protected]>

sinkap closed this Jun 14, 2017

sinkap reopened this Jun 14, 2017

sinkap force-pushed the master branch from 6f40d93 to a84dd23 Compare June 14, 2017 21:41

joelagnel closed this Jun 14, 2017

valschneider pushed a commit to valschneider/trappy that referenced this pull request Sep 28, 2018

Merge pull request ARM-software#249 from bjackman/cpuidle-fix

663a176

Update trappy and fix cpu_idle

		@@ -0,0 +1,33 @@
		# Copyright 2017 ARM Limited, Google and contributors

systrace parsing new edition #249

systrace parsing new edition #249

Uh oh!

Conversation

joelagnel commented Jun 12, 2017

Uh oh!

bjackman commented Jun 12, 2017

Uh oh!

bjackman commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelagnel commented Jun 12, 2017

Uh oh!

bjackman commented Jun 12, 2017

Uh oh!

sinkap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinkap commented Jun 14, 2017

Uh oh!

joelagnel commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bjackman commented Jun 12, 2017 •

edited

Loading