----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1644/#review3932 -----------------------------------------------------------
I understand the desire to avoid extra getattr lookups, but the way you're going about it seems rather confusing (and I believe could be done just as efficiently in a more clear manner). Also, it's hard for me to suggest improvements because the optimizations are cluttered with extra code and spread across diffs. src/python/m5/stats/__init__.py <http://reviews.gem5.org/r/1644/#comment3809> This seems like a bugfix that belongs in a different patch src/python/m5/stats/sql.py <http://reviews.gem5.org/r/1644/#comment3808> This seems like it belongs in a different patch. src/python/m5/stats/sql.py <http://reviews.gem5.org/r/1644/#comment3806> seems to me that this sort of thing belongs in the stat object, not here. Like a stat.getValue function. I assume that the info is not available, so you can't use __getattr__ and it must be a function. That's fine, but this seems like the wrong place to do this. src/python/m5/stats/sql.py <http://reviews.gem5.org/r/1644/#comment3810> This seems like a pretty bad place to put this code. Why does statContainer calculate the value instead of the stat itself? Is it because the container has the info? If so, it seems like you should have the container calculate the value instead of doing it here. src/python/m5/stats/sql.py <http://reviews.gem5.org/r/1644/#comment3807> Seems like you should be looping over the values and recording if they're all zero, and if there are any NaN values instead of building an intermediate list (which is comparatively expensive). You can also exit early. hasNonNan = False hasNonZero = False for x in theValue: if not isnan(x): hasNonNan = True if hasNonZero: break if x: hasNonZero = True if hasNonNan: break - Nathan Binkert On Jan. 15, 2013, 10:34 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1644/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2013, 10:34 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9497:9835f2157f30 > --------------------------- > stats: Optimise SQL stats output to improve speed and file size > > This patch optimises the SQL stats. It does this by only writing to the stats > database if and when the stat is non-zero and non-NaN and the corresponding > flag is set. It also removes unnecessary calls to getattr and hasattr by > accessing the values directly. Finally, it only calculates the total for a > formula if necessary > > > Diffs > ----- > > src/python/m5/stats/__init__.py 5532a1642108 > src/python/m5/stats/sql.py PRE-CREATION > > Diff: http://reviews.gem5.org/r/1644/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
