trappy: Add support for 'print:' ftrace fallback event [v2] #280

douglas-raillard-arm · 2018-08-21T15:23:06Z

Respin of:
#276

Changelog:

Modified the exception system when accessing the wrong attribute. It is now handled by GenericFTrace.__getattr__ instead of introducing an new class. That also allows catching issues earlier.
Removed a RuntimeError that could apparently not happen (i.e. having the class accessible as an attribute named with the unique_word, at the same time as being available under the class name, with the class name being different from the unique_word)

[1] Event parser objects are stored in the per-scope (i.e. sched/thermal/dynamic) dicts at import time via register_parser. These dicts are keyed using the 'name' attribute of the event parsers. [2] During trace construction the class_definitions attribute is initially created from the per-scope objects created in [1]. [3] Then, for each event requested by the 'events' parameter, the event class is added to class_definitions (they may already be present, having been added in [2], in which case this _should_ be a nop). Some event classes may have a `name` that is different from the actual event name that is printed in the trace (i.e. `unique_word`), for example the 'print' event is parsed by a parser class (to be added) whose 'name' is 'print_' in order to avoid keyword collisions. Since step [1] currently uses `name` to index class_definitions but step [3] uses the actual printed event name (i.e. `unique_word`), the expected no-op I mentioned above actually results in two instances of the same event parser being present in class_definitions. This leads to missing data, where one instance of the event parser has a populated DataFrame and the other does not, and causes great angst, woe and distress in developers. So instead always use `name` to index class_definitions. This bug was being taken advantage of by the systrace/sched tests - the sched_contrib_scale_f event is currently parsed by the SchedContribScaleFactor dynamic ftrace class, whose 'name' attribute (set via trappy.dynamic._get_name) is currently sched_contrib_scale_factor but whose uniqe_word is sched_contrib_scale_f. Therefore ftrace objects haave an attribute with both names. With this commit, the 'sched_contrib_scale_f' attribute is removed, so the test is updated to use 'sched_contrib_scale_factor'

If we have a bug like this we should give up and fix the bug before trying to do anything else, otherwise things will break in desperately confusing ways.

valschneider · 2018-08-21T15:42:06Z

trappy/ftrace.py

@@ -465,6 +472,33 @@ def __parse_trace_file(self, trace_file):
            raise ValueError('Failed to parse ftrace file {}:\n{}'.format(
                trace_file, str(e)))

+    def __getattr__(self, attr):


IIRC this is something that's not made explicit in the commit message - this raises an exception when trying to get a dataframe for an event by specifying its unique_word instead of its name. #276 instead raised an exception at parse time, which is different, unless I missed something?

I may need to update the commit message, but the behavior is largely the same. #276 did not raise exception at parse time, it was only when an attribute was accessed on the thing retrieved on the trace. Now, it fails straight away when you try to get "the thing":

# Used to work trace.my_unique_word # Used to raise trace.my_unique_word.data_frame # Now raises trace.my_unique_word trace.my_unique_word.data_frame

Oh ok, I missed the set_attr at the end of __add_warnings in #276. LGTM then.

JaviMerino

Three minor nits.

JaviMerino · 2018-08-22T14:36:41Z

trappy/ftrace.py

+                        attr=attr,
+                        name=name
+                    ))
+        raise AttributeError('{} object has no attribute {}'.format(


Don't try to imitate the exception that was raised, just let the parent handle it was happening we added this:

return super(GenericFTrace, self).__getattribute__(attr)

Right, object.getattr does not exist but getattribute does

JaviMerino · 2018-08-22T14:38:39Z

tests/test_fallback.py

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


JaviMerino · 2018-08-22T14:52:28Z

trappy/fallback.py

+class FallbackEvent(Base):
+    """
+    Parse free-form events that couldn't be matched with more specific unique
+    words This class is always used as a fallback if nothing more specific could


'.' between "words" and "This"

We used to have a bug where when you had a known event whose 'name' attribute != its 'unique_word', and you specified it explicitly in the 'events' param, we had two attributes - one for the 'name' and one for the 'unique_word', and it was undefined which would be populated. Now we just have the attribute from the 'name'. If there is any code out there that was relying on this bug (i.e. accessing the 'unique_word' atribute), this will tell them what they need to do to fix their code. Co-author: Douglas Raillard <douglas.raillard@arm.com>

Currently if you specify an event in the `events` by the `name` attribute of its parser class, __add_events will add that name to the class_definitions dict. For events in the 'thermal' and 'sched' scopes, that is then overwritten, but for events in the dynamic_classes dict (where scope='all'), this is not the case. That means that a specialised event parser class will be replaced with a basic dynamic parser. Fix that by checking the 'name' field in known_events.

This is just like tracing_mark_write but with a different name. It poses a minor difficulty as its name is a Python 2 keyword (so 'ftrace.print.data_frame' is a syntax error) - this is solved by appending an underscore to the name, as suggested by PEP8. A base class is added which encapsulates the fallback behaviour for both tracing_mark_write and print.

Brendan Jackman added 2 commits August 21, 2018 13:41

GenericFTrace: Add warning for duplicate parser classes

ee56bfc

If we have a bug like this we should give up and fix the bug before trying to do anything else, otherwise things will break in desperately confusing ways.

douglas-raillard-arm requested review from JaviMerino and valschneider August 21, 2018 15:23

douglas-raillard-arm mentioned this pull request Aug 21, 2018

trappy: Add support for 'print:' ftrace fallback event #276

Closed

valschneider reviewed Aug 21, 2018

View reviewed changes

valschneider approved these changes Aug 21, 2018

View reviewed changes

douglas-raillard-arm mentioned this pull request Aug 23, 2018

Python 3 #281

Merged

JaviMerino suggested changes Aug 23, 2018

View reviewed changes

Brendan Jackman added 3 commits September 3, 2018 13:37

douglas-raillard-arm force-pushed the brendan_pr branch from 417e4cb to 2d8e485 Compare September 3, 2018 12:39

JaviMerino approved these changes Sep 4, 2018

View reviewed changes

JaviMerino merged commit 61d0320 into ARM-software:master Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trappy: Add support for 'print:' ftrace fallback event [v2] #280

trappy: Add support for 'print:' ftrace fallback event [v2] #280

douglas-raillard-arm commented Aug 21, 2018 •

edited

Loading

valschneider Aug 21, 2018

douglas-raillard-arm Aug 21, 2018

valschneider Aug 21, 2018

JaviMerino left a comment

JaviMerino Aug 22, 2018

douglas-raillard-arm Aug 24, 2018

JaviMerino Aug 22, 2018

douglas-raillard-arm Sep 3, 2018

JaviMerino Aug 22, 2018

douglas-raillard-arm Sep 3, 2018

		@@ -0,0 +1,62 @@

		# Copyright 2017 ARM Limited, Google and contributors

trappy: Add support for 'print:' ftrace fallback event [v2] #280

trappy: Add support for 'print:' ftrace fallback event [v2] #280

Conversation

douglas-raillard-arm commented Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaviMerino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglas-raillard-arm commented Aug 21, 2018 •

edited

Loading