src/python/m5/stats/display.py (Diff revision 1)

Why do we need to re-create noNan?

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:11 p.m. (Jan. 28, 2013, 7:11 p.m.)

   We don't and it has been removed.

src/python/m5/stats/__init__.py (Diff revision 1)

Since you are changing this function call, should not the function definition also change?

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:11 p.m. (Jan. 28, 2013, 7:11 p.m.)

   Sorry, not quite sure how this got into this patch. Removed it now.

I could be wrong about the getattr thing, but this patch seems like it needs work.

src/python/m5/stats/__init__.py (Diff revision 1)

seems like an unrelated fix.

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   Removed. Somehow it crept into one patch and got removed in the other. 

src/python/m5/stats/display.py (Diff revision 1)

I'm probably missing context, but this seems superfluous.  You do the same thing below.

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   Removed this one.

src/python/m5/stats/display.py (Diff revision 1)

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.

Show all issues

src/python/m5/stats/display.py (Diff revision 1)

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.

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   I have removed the exception, and try and be a bit more intelligent about it.

src/python/m5/stats/display.py (Diff revision 1)

I don't think that you're improving anything here at all.  Are you sure you understand __getattr__?

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   This is not trying to avoid the __getattr__ call. Instead, it is trying to see if the total of the evaluated formula has been passed in from C++ to avoid calculating it in python. This results in a significant speedup. However, you are correct and I have changed the code accordingly.

src/python/m5/stats/info.py (Diff revision 1)

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?

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   Removing these asserts increased the stats dumping speed by a significant amount. This is because the scalar() and vector() functions are called extremely often.

src/python/m5/stats/info.py (Diff revision 1)

Poor use of __getattr__

Show all issues

1. Sascha Bischoff Jan. 28, 2013, 7:13 p.m. (Jan. 28, 2013, 7:13 p.m.)

   I hate to point out that you are the original author of this line. :)
   
   That said, how do you propose we change it?

src/python/m5/stats/info.py (Diff revision 1)

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.

Show all issues

src/python/m5/stats/info.py (Diff revision 1)

I'm not objecting to their removal, since in this case, it is probably overkill.

I'm just suggesting a general speedup plan for you guys.  When you bytecode compile python as optimized code, all it does is remove the assert statements.  So, in the build process where we compile all of the python objects (src/Sconscript, embedPyFile), we could compile optimized for m5.fast and that would remove all assertions.  (All that said, it's non trivial since I think you need to run python with -O to be able to compile optimized modules.)

Show all issues

src/python/m5/stats/info.py (Diff revision 1)

I never said I was perfect!  Unfortunately, there is tons of my crappy code in here.

Anyway,  I wasn't talking about this specific line, but the new lines below with the hasattr.  __getattr__ does more or less what you're doing there.  Get rid of the hasattr stuff, and further below, where you set "self.calculatedValue", change it to "self._the_value"  This will allow you to avoid unnecessary calls to __getattr__ (the way you've written it, you'll call it every time you try to get _the_value).

Show all issues

src/python/m5/stats/info.py (Diff revision 1)

Same as above

Show all issues

src/python/m5/stats/display.py (Diff revision 3)

try/except may be faster here.  I haven't benchmarked that though.

Show all issues

1. Sascha Bischoff April 24, 2013, 10:07 p.m. (April 24, 2013, 10:07 p.m.)

   I had a look, seems like try/except is very slightly slower in this case. Very marginal though.

2. Nathan Binkert April 25, 2013, 2:20 a.m. (April 25, 2013, 2:20 a.m.)

   Huh.  I must admit that I'm surprised.  I'd have thought that the two lookups would have been more expensive.  Perhaps failing that call is the common case.  The other idiom I use is:
   
   total = getattr(self, "total", None)
   if total is None:
      total = foo....
   
   I just don't know enough about CPython performance (beyond that it doesn't have any :)
   
   (I'm not trying to get you to change anything, just giving you ideas on improving the performance.)

src/python/m5/stats/display.py (Diff revision 3)

Don't catch all exceptions.  This can swallow things like KeyboardInterrupt.  Be explicit about catching exceptions.  This seems like it should catch AttributeError and or IndexError only.

Show all issues

src/python/m5/stats/display.py (Diff revision 3)

Similarly, this should only catch AttributeError.

Show all issues