----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1643/#review3908 -----------------------------------------------------------
I could be wrong about the getattr thing, but this patch seems like it needs work. src/python/m5/stats/__init__.py <http://reviews.gem5.org/r/1643/#comment3782> seems like an unrelated fix. src/python/m5/stats/display.py <http://reviews.gem5.org/r/1643/#comment3776> I'm probably missing context, but this seems superfluous. You do the same thing below. src/python/m5/stats/display.py <http://reviews.gem5.org/r/1643/#comment3775> fyi sum(x for x in self.value if not isnan(x)) is faster than what you have because it does not create a temporary list. you could also just replace the square brackets with parens to turn the list into a generator expression. src/python/m5/stats/display.py <http://reviews.gem5.org/r/1643/#comment3777> Imho, it is very bad form to catch all exceptions here. You will for example, trap a KeyboardInterrupt exception which seems like a bad idea. Catch the actual exception(s) that you're worried about. To catch more is a bug. src/python/m5/stats/display.py <http://reviews.gem5.org/r/1643/#comment3781> I don't think that you're improving anything here at all. Are you sure you understand __getattr__? src/python/m5/stats/info.py <http://reviews.gem5.org/r/1643/#comment3785> If the asserts really aren't needed, it's fine to remove them, but an alternative to removing asserts, is to bytecode compile python optimized in m5.fast? src/python/m5/stats/info.py <http://reviews.gem5.org/r/1643/#comment3780> Poor use of __getattr__ src/python/m5/stats/info.py <http://reviews.gem5.org/r/1643/#comment3779> I think that you're using __getattr__ wrong. __getattr__ is only called if the attribute is missing, so if you were to set _the_total instead of calculatedTotal, you wouldn't even need the initial check that you do here. - Nathan Binkert On Jan. 15, 2013, 10:33 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1643/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2013, 10:33 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9496:d3f96871d795 > --------------------------- > stats: Optimise the text-based stats output to improve speed > > The text-based stats output is slow than the original C++ output. This patch > addresses some of those issues by removing: > > Asserts that are not required > stats.ValueProxy, as it is no longer required > Unnecessary calls to __getattr__ and __hasattr__ > > It also changes the formula calculation to only calculate the values and > totals > of formulas if they have not been passed in from the C++. This allows other > formulas to be added, whilst ensuring that the stats output is sufficiently > fast. > > Testing has shown a 6x - 8x increase in stats output speed. > > > Diffs > ----- > > src/python/m5/stats/__init__.py 5532a1642108 > src/python/m5/stats/display.py PRE-CREATION > src/python/m5/stats/info.py PRE-CREATION > > Diff: http://reviews.gem5.org/r/1643/diff/ > > > Testing > ------- > > Ensure that the output files are identical > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
