sim: move iterating over SimObjects into Python.

Information
Submitter:	Steve Reinhardt
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

sim: move iterating over SimObjects into Python.

Testing Done

Diff:

Revision 2 (+37 -122)

Show changes

	src/python/m5/SimObject.py
	src/python/m5/core.py
	src/python/m5/simulate.py
	src/python/m5/stats.py
	src/python/swig/core.i
	src/python/swig/pyobject.hh
	src/python/swig/sim_object.i
	src/sim/sim_object.hh
	1 more

src/python/m5/SimObject.py (Diff revision 2)

I wonder if Children doesn't quite describe it right since children doesn't obviously imply recursion.  iterDescendants?  (I don't have strong feelings here.)

Steve Reinhardt June 28, 2010, 3:41 p.m. (June 28, 2010, 3:41 p.m.)

I don't disagree, but don't have a lot of great alternatives.  iterDescendants is too long.  iterTree?

Nathan Binkert June 28, 2010, 4:15 p.m. (June 28, 2010, 4:15 p.m.)

could just do descendants().  You could even go overboard and make it an @property, just so you could say "for obj in root.descendants: obj.foo()".  Actually, I kinda like that.

Steve Reinhardt July 1, 2010, 9:06 a.m. (July 1, 2010, 9:06 a.m.)
```
iterAll is my current favorite...
```

src/python/m5/SimObject.py (Diff revision 2)

Not sure if it is worth it, but we could add the following to __getattr__(self, attr):

try:
    return getattr(self._ccObject, attr)
except AttributeError:
    pass

There are two failures, _ccObject doesn't exist yet, and attr is not found.  I think both are handled by AttributeError.  Anyway it's just an idea since the code looks repetitive.  Could also do:
if attr in ('init', 'regStats' .....):
   return getattr(self._ccObject, attr)

Just less code and I know that you like less code :)

Steve Reinhardt June 28, 2010, 3:41 p.m. (June 28, 2010, 3:41 p.m.)
```
Sounds good to me... I'll give it a try.
```

Steve Reinhardt July 1, 2010, 9:04 a.m. (July 1, 2010, 9:04 a.m.)

I did this to be more parallel with the rest of __getattr__:

        # If the attribute exists on the C++ object, transparently
        # forward the reference there.  This is typically used for
        # SWIG-wrapped methods such as init(), regStats(),
        # regFormulas(), resetStats(), and startup().
        if self._ccObject and hasattr(self._ccObject, attr):
            return getattr(self._ccObject, attr)

It seems to work.

src/python/m5/stats.py (Diff revision 2)

Not sure if it matters at all, but you've changed the order of reset.  Currently, we reset stats and then reset objects.  I hope that nothing depends on this order, but I thought I'd mention it.

Steve Reinhardt July 1, 2010, 9:03 a.m. (July 1, 2010, 9:03 a.m.)

Since this ordering was implicit in the order that reset callbacks are registered and invoked--it took me a while to prove to myself that you were right--I'd sure hope nothing depended on it.  I've seen no evidence of it either.

src/sim/sim_object.cc (Diff revision 2)

I don't care much, but do we want to create these as DPRINTFs in the Python code?  Could help in stats debugging.

Diff:

Revision 3 (+49 -167)

Show changes

	src/python/m5/SimObject.py
	src/python/m5/core.py
	src/python/m5/simulate.py
	src/python/m5/stats.py
	src/python/swig/core.i
	src/python/swig/pyobject.hh
	src/python/swig/sim_object.i
	src/sim/sim_object.hh
	1 more

src/python/m5/SimObject.py (Diff revision 3)

Seems like you could have a lot less code if you had a sorted=False parameter to iterChildren.  You could also rely on people just calling (or implement it by doing) sorted(iterChildren(), key=lambda c: c.name)

src/python/m5/simulate.py (Diff revision 3)

Why exactly is this more deterministic?  It seems that if unproxying can be non-deterministic, we should flag that as an error, no?

src/python/m5/simulate.py (Diff revision 3)

I'd say for easier diffing.

src/python/swig/sim_object.i (Diff revision 3)

How far are we from just doing a %include of sim_object.hh?

src/sim/sim_object.cc (Diff revision 3)

I like this!

src/python/m5/SimObject.py (Diff revision 3)

I'm not sure you'd save that many LOC, but it might be a nicer interface.  Also, since you cast some doubt on one of the two places it's used, forcing the caller to use sorted() doesn't seem so bad.

src/python/m5/simulate.py (Diff revision 3)

Good point... I just copied the logic and the comment from SimObject.py line 684.  I agree it seems unnecessary.

src/python/m5/simulate.py (Diff revision 3)

Determinism across minor code changes, so basically the same thing... but yeah.

src/python/swig/sim_object.i (Diff revision 3)

Beats me, it didn't cross my mind...

I tried unproxying without sorting and the regressions passed, so it looks like you were right that it's unnecessary.  I then got rid of iterSortedChildren() and replaced the one remaining use with
  for obj in sorted(root.iterChildren(), key=lambda o: o.path()):
      obj.print_ini(ini_file)
I can upload a new diff if you want but it seemed like overkill.

As far as the naming, I guess iterChildren has kind of grown on me; the @property thing does seem like going overboard, but calling it descendants() might not be too bad.  How much benefit is there in having 'iter' in there so you know it's an iterator?  I'd be interested in having people other than Nate weigh in on this, since while I consider his opinions valuable, his perspective is not necessarily typical ;-).

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff:

Diff: