| Message ID | 20230502092339.27341-6-mads@ynddal.dk |
|---|---|
| State | New |
| Headers | show |
| Series | simpletrace: refactor and general improvements | expand |
On Tue, May 02, 2023 at 11:23:32AM +0200, Mads Ynddal wrote: > From: Mads Ynddal <m.ynddal@samsung.com> > > Instead of explicitly calling `begin` and `end`, we can change the class > to use the context-manager paradigm. This is mostly a styling choice, > used in modern Python code. But it also allows for more advanced analyzers > to handle exceptions gracefully in the `__exit__` method (not > demonstrated here). > > Signed-off-by: Mads Ynddal <m.ynddal@samsung.com> > --- > scripts/simpletrace.py | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py > index 7444a6e090..10ca093046 100755 > --- a/scripts/simpletrace.py > +++ b/scripts/simpletrace.py > @@ -121,12 +121,12 @@ def read_trace_records(event_mapping, event_id_to_name, fobj): > > yield rec > > -class Analyzer(object): > +class Analyzer: > """A trace file analyzer which processes trace records. > > - An analyzer can be passed to run() or process(). The begin() method is > - invoked, then each trace record is processed, and finally the end() method > - is invoked. > + An analyzer can be passed to run() or process(). The __enter__() method is > + invoked when opening the analyzer using the `with` statement, then each trace > + record is processed, and finally the __exit__() method is invoked. Bearing in mind compatibility with existing simpletrace analysis scripts, how about the following default method implementations? def __enter__(self): self.begin() def __exit__(self, exc_type, exc_val, exc_tb): if exc_type is None: self.end() return False Now simpletrace.py can switch to using the context manager and new scripts can implement __enter__()/__exit__(), while old scripts continue to work. > > If a method matching a trace event name exists, it is invoked to process > that trace record. Otherwise the catchall() method is invoked. > @@ -152,19 +152,25 @@ def runstate_set(self, timestamp, pid, new_state): > ... > """ > > - def begin(self): > + def __enter__(self): > """Called at the start of the trace.""" > - pass > + return self > > def catchall(self, event, rec): > """Called if no specific method for processing a trace event has been found.""" > pass > > - def end(self): > + def __exit__(self, _type, value, traceback): > """Called at the end of the trace.""" > pass > > -def process(events, log, analyzer, read_header=True): > + def __call__(self): > + """Fix for legacy use without context manager. > + We call the provided object in `process` regardless of it being the object-type or instance. > + With this function, it will work in both cases.""" > + return self > + > +def process(events, log, analyzer_class, read_header=True): Please don't change the function signature since this is a public method and we should avoid breaking existing callers when possible. Instead of: with analyzer_class() as analyzer: we can use: with analyzer: ...
> > Bearing in mind compatibility with existing simpletrace analysis > scripts, how about the following default method implementations? > > def __enter__(self): > self.begin() > > def __exit__(self, exc_type, exc_val, exc_tb): > if exc_type is None: > self.end() > return False > > Now simpletrace.py can switch to using the context manager and new > scripts can implement __enter__()/__exit__(), while old scripts continue > to work. I was considering the same, but I was worried about double initialization if someone used both the context manager as well as calling .begin(). Should we add a guard, that prohibits this? Otherwise, we could also keep begin()/end() in Analyzer, and then make Analyzer2 a context manager? > > Please don't change the function signature since this is a public method > and we should avoid breaking existing callers when possible. > > Instead of: > > with analyzer_class() as analyzer: > > we can use: > > with analyzer: > ... I didn't think of that. Let's do this, if we keep the context manager.
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py index 7444a6e090..10ca093046 100755 --- a/scripts/simpletrace.py +++ b/scripts/simpletrace.py @@ -121,12 +121,12 @@ def read_trace_records(event_mapping, event_id_to_name, fobj): yield rec -class Analyzer(object): +class Analyzer: """A trace file analyzer which processes trace records. - An analyzer can be passed to run() or process(). The begin() method is - invoked, then each trace record is processed, and finally the end() method - is invoked. + An analyzer can be passed to run() or process(). The __enter__() method is + invoked when opening the analyzer using the `with` statement, then each trace + record is processed, and finally the __exit__() method is invoked. If a method matching a trace event name exists, it is invoked to process that trace record. Otherwise the catchall() method is invoked. @@ -152,19 +152,25 @@ def runstate_set(self, timestamp, pid, new_state): ... """ - def begin(self): + def __enter__(self): """Called at the start of the trace.""" - pass + return self def catchall(self, event, rec): """Called if no specific method for processing a trace event has been found.""" pass - def end(self): + def __exit__(self, _type, value, traceback): """Called at the end of the trace.""" pass -def process(events, log, analyzer, read_header=True): + def __call__(self): + """Fix for legacy use without context manager. + We call the provided object in `process` regardless of it being the object-type or instance. + With this function, it will work in both cases.""" + return self + +def process(events, log, analyzer_class, read_header=True): """Invoke an analyzer on each event in a log.""" if read_header: read_trace_header(log) @@ -203,15 +209,15 @@ def build_fn(analyzer, event): # Just arguments, no timestamp or pid return lambda _, rec: fn(*rec[3:3 + event_argcount]) - analyzer.begin() - fn_cache = {} - for rec in read_trace_records(event_mapping, event_id_to_name, log): - event_num = rec[0] - event = event_mapping[event_num] - if event_num not in fn_cache: - fn_cache[event_num] = build_fn(analyzer, event) - fn_cache[event_num](event, rec) - analyzer.end() + with analyzer_class() as analyzer: + fn_cache = {} + for rec in read_trace_records(event_mapping, event_id_to_name, log): + event_num = rec[0] + event = event_mapping[event_num] + if event_num not in fn_cache: + fn_cache[event_num] = build_fn(analyzer, event) + fn_cache[event_num](event, rec) + def run(analyzer): """Execute an analyzer on a trace file given on the command-line.