ruby: consumer: pass on the current tick to wakeup() function

Information
Submitter:	Nilay Vaish
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11032:95ddf324be85
---------------------------
ruby: consumer: pass on the current tick to wakeup() function

This is to avoid calling clockEdge() sometimes.

Testing Done

I don't quite follow this patch. You say "This is to avoid calling clockEdge() sometimes.", is there another patch coming that removes some calls to clockEdge()? Or am I just missing them in this patch.

It doesn't seem very helpful to me to change this function, but maybe I'm missing something.

src/mem/ruby/network/garnet/flexible-pipeline/Router.cc (Diff revision 1)

Misspelling: currTick->curTick

Nilay Vaish Aug. 10, 2015, 9:02 a.m. (Aug. 10, 2015, 9:02 a.m.)
```
Done.
```

This patch is simply changing the API without making use of the new argument. Please post a full patch showing how this will be used, so proper reviews can be done.

Nilay Vaish Aug. 13, 2015, 12:26 p.m. (Aug. 13, 2015, 12:26 p.m.)

Let me give you an example from another patch: http://reviews.gem5.org/r/3009. Look at the portion that changes the file: src/mem/ruby/network/garnet/fixed-pipeline/InputUnit_d.cc.  Earlier, on line 70, we were calling curCycle() to get the current time in the wakeup() function.  Request 3009 would have converted that to clockEdge().  But since this patch (3005) provides the current time as an argument to the wakeup() function, we need not call clockEdge() at all.

Tony Gutierrez Aug. 13, 2015, 5:05 p.m. (Aug. 13, 2015, 5:05 p.m.)

Is there a reason to do this though, other than to avoid calling clockEdge() inside of wakeup()? If not then I don't think the API should changed, i.e., those details should be hidden inside wakeup() so callers don't need to do it.

Nilay Vaish Aug. 13, 2015, 8:01 p.m. (Aug. 13, 2015, 8:01 p.m.)

This is a performance optimization. Calling ClockedObject::clockEdge() is more costly compared to calling Event::when().  There are only four places in ruby where wakeup() is called.  So, not many changes to the callers.

Any more comments before it push this to the repo?

Jason Lowe-Power Sept. 9, 2015, 11:54 a.m. (Sept. 9, 2015, 11:54 a.m.)

I agree with Tony that this seems like a lot of changes for a small benefit. At a minumum I would like to know what the performance benefit actually is (10% faster, totally worth it, 0.5% faster, I'm not sure). Additionally, since this really doesn't make any difference until http://reviews.gem5.org/r/3009 is submitted, I would like to at least wait until after that patch.

Tony Gutierrez Sept. 10, 2015, 11:37 a.m. (Sept. 10, 2015, 11:37 a.m.)

If you're going for performance with this patch, please quantify the benefit to support your claim. If it's not significant, I would prefer to not ship this.

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Status: Discarded