ruby: consumer: move away from Consumer class

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

Description

Changeset 11136:c4c3b2f0e36b
---------------------------
ruby: consumer: move away from Consumer class

Objects in ruby memory system typically inherit from the Consumer class that
provides support for scheduling events.  The Consumer class maintains a std::set
of times at which events have been posted by the Derived class object.  Typically
this causes a lot of overhead.  Secondly, the objects schedule events that are
very coarse grained.  This patch reduces ruby memory system's reliance on the Consumer
class.

The patch changes the objects in the Network and the generated code for Controllers
in a significant way.  The generated controllers would now schedule events for
individual input ports and not for the entire controller itself.  Similarly, PerfectSwitch
and Throttle would now schedule events on individual message buffers and not for the
entire object.  This avoids looking at buffers that do not have any message pending.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Is the majority of the performance gain just from removing the alreadyScheduled() calls and dynamically allocated events? Or something else? ...	Joel Hestness	Aug. 17, 2015, 8:16 p.m.	Dropped
Why does the MessageBuffer now have a pointer to the receiver's wakeupEvent? I feel that it be the responsibility of ...	Joel Hestness	Sept. 1, 2015, 5:53 p.m.	Resolved
Please use a longer name than em. This is not a local variable, but rather it is now a very ...	Brad Beckmann	Sept. 18, 2015, 5:31 p.m.	Resolved
Does this make sense to set the event manager but not set the event associated with the event manager? I ...	Brad Beckmann	Aug. 14, 2015, 4:29 p.m.	Dropped

Nice work tracking down 15-20% sim time! I WANT THAT!

It seems like this patch is trying to do a couple fairly orthogonal things: (1) improve performance by removing the Consumer class' scheduled event lookups and dynamically allocated events, and (2) move wakeup event scheduling responsibility from the Consumers into MessageBuffers. I really like the first aim, but I don't agree with the second. More below:

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)

The patch will ultimately eliminate the Consumer class.  So both (1) and (2) will happen.  (2) is incomplete in the sense that

the responsibility is being shared by both the message buffer and the receiver.

Joel Hestness Aug. 19, 2015, 10:13 a.m. (Aug. 19, 2015, 10:13 a.m.)

Ok, I see what you're describing. Thanks for clarifying.
My concern is that the resulting software architecture after removal of the Consumer class is going to be inconsistent with gem5 best practices. Specifically, components should be scheduling their own events (which may be triggered by receiving messages from other components). In contrast, the architecture that this patch proposes gives the event scheduling responsibility to the MessageBuffer that is feeding the a receiver component.
I feel that we should be aiming for the following: if the MessageBuffer has a message that the receiver can consume, it should signal the presence of the message to the receiver, and the receiver should schedule its own event to handle that incoming message. Event scheduling can still be handled inside the receiver; Separate ports, or master IDs, etc. are used throughout gem5 to disambiguate the sender and appropriate events to schedule when a component may be a slave to multiple masters.

src/mem/ruby/common/Consumer.cc (Diff revision 1)

Is the majority of the performance gain just from removing the alreadyScheduled() calls and dynamically allocated events? Or something else?

If the gain is from axing alreadyScheduled() and dynamic events, then that's all this patch should change.

Show all issues

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)
```
The majority gain is from making the events fine grained.
```
Joel Hestness Aug. 19, 2015, 10:13 a.m. (Aug. 19, 2015, 10:13 a.m.)
```
Ok, thanks for clarifying. I've dropped this issue.
```

src/mem/ruby/network/MessageBuffer.cc (Diff revision 1)

Why does the MessageBuffer now have a pointer to the receiver's wakeupEvent?
I feel that it be the responsibility of the Consumer (receiver) to figure out when it will be able to pop something from the buffer. The MessageBuffer sorts its in-flight messages by time in order to signal to the receiver the first time at which the receiver is allowed to pop from the buffer. Seems like it would make more sense for the MessageBuffer to call a function on the receiver pointer to tell it when it can do so (e.g. scheduleEventAbsolute).

Show all issues

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)

The receiver has multiple wakeup events, one per incoming message buffer. So more information needs to be provided
by the message buffer on what event to be scheduled. That information is the pointer to the wakeup event
corresponding to this particular message buffer.

Thanks Nilay for sending out this patch to start the discussion.  Unlike some of the other recent patches you posted, this one has obvious benefits and should not adversly impact too much downstream code.  Having the Consumer class use AutoDelete Events was never meant to be the long-term solution.  It was simply the easiest way for us to complete the initial merger of GEMS and M5.  I'm glad to see this improvement.
So can you comment on whether you see this as an eventual set to eliminate the Consumer class?

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)
```
That's the aim.
```

src/mem/ruby/network/MessageBuffer.hh (Diff revision 1)

Please use a longer name than em. This is not a local variable, but rather it is now a very important member for MessageBuffer. It deserves a more descriptive name and yes, I understand that there may be bad precedent in other parts of the code. For a variable as important this the EventManger, we should make sure it is easy to search for.

Show all issues

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)
```
OK.
```

src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc (Diff revision 1)

Does this make sense to set the event manager but not set the event associated with the event manager? I probably do not fully appreciate the EventManager design, but I thought it existed just to make sure that a single event was only scheduled on a single event queue. It seems like the code requires the event to be set as well.

Perhaps this is why you say the patch is incomplete and that Garnet will not work.

Show all issues

Nilay Vaish Aug. 14, 2015, 4:29 p.m. (Aug. 14, 2015, 4:29 p.m.)
```
Garnet is still incomplete.
```

Summary:

-	ruby: consumer: move away from Consumer class
+	ruby: garnet: refactor code in network links

Description:

~		Changeset 10948:12a8ecefe478
	~	Changeset 11135:5e6d088f2e7c

~		ruby: consumer: move away from Consumer class
	~	ruby: garnet: refactor code in network links
-
-		Objects in ruby memory system typically inherit from the Consumer class that
-		provides support for scheduling events. The Consumer class maintains a std::set
-		of times at which events have been posted by the Derived class object. Typically
-		this causes a lot of overhead. Secondly, the objects schedule events that are
-		very coarse grained. This patch reduces ruby memory system's reliance on the Consumer
-		class.
-
-		The patch changes the objects in Simple Network and the generated code for Controllers
-		in a significant way. The generated controllers would now schedule events for
-		individual input ports and not for the entire controller itself. Similarly, PerfectSwitch
-		and Throttle would now schedule events on individual message buffers and not for the
-		entire object. This avoids looking at buffers that do not have any message pending.
-
-		This patch is incomplete in the sense that Garnet would not work if this patch is committed.
-		Fixing Garnet would take some time due to its own peculiarities. The patch is being
-		put up more from a discussion point of view before I embark on changing Garnet.
-
-		Overall we see about 15-20% improvement in performance of the only ruby fs regression
-		test we have in our suite.

Diff:

Revision 2 (+15 -30)

Show changes

	src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.hh
	src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.cc
	src/mem/ruby/network/garnet/flexible-pipeline/NetworkLink.cc

Summary:

-	ruby: garnet: refactor code in network links
+	ruby: consumer: move away from Consumer class

Description:

~		Changeset 11135:5e6d088f2e7c
	~	Changeset 11136:c4c3b2f0e36b

~		ruby: garnet: refactor code in network links
	~	ruby: consumer: move away from Consumer class
	+
	+	Objects in ruby memory system typically inherit from the Consumer class that
	+	provides support for scheduling events. The Consumer class maintains a std::set
	+	of times at which events have been posted by the Derived class object. Typically
	+	this causes a lot of overhead. Secondly, the objects schedule events that are
	+	very coarse grained. This patch reduces ruby memory system's reliance on the Consumer
	+	class.
	+
	+	The patch changes the objects in the Network and the generated code for Controllers
	+	in a significant way. The generated controllers would now schedule events for
	+	individual input ports and not for the entire controller itself. Similarly, PerfectSwitch
	+	and Throttle would now schedule events on individual message buffers and not for the
	+	entire object. This avoids looking at buffers that do not have any message pending.

Diff:

Revision 3 (+380 -333)

Show changes

	src/mem/ruby/network/MessageBuffer.hh
	src/mem/ruby/network/MessageBuffer.cc
	src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.hh
	src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc
	src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.hh
	src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.cc
	src/mem/ruby/network/garnet/fixed-pipeline/flitBuffer_d.hh
	src/mem/ruby/network/garnet/fixed-pipeline/flitBuffer_d.cc
	22 more

Can anyone see the latest diff for this patch? At least for me I encounter the following error in both IE and Chrome:

The patch to 'src/mem/ruby/network/MessageBuffer.hh' didn't apply cleanly. The temporary files have been left in '/tmp/reviewboard.h2UJk2' for debugging purposes.
patch returned:

 This may be a bug in the software, a temporary outage, or an issue
 with the format of your diff.

 Please try again, and if you still have trouble,
 contact support

Nilay Vaish Sept. 24, 2015, 7:59 a.m. (Sept. 24, 2015, 7:59 a.m.)

I am going to discard this patch. A new request has been posted.

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Issue Summary

Summary:

Description:

Changeset 10948:12a8ecefe478

Changeset 11135:5e6d088f2e7c

Diff:

Summary:

Description:

Changeset 11135:5e6d088f2e7c

Changeset 11136:c4c3b2f0e36b

Diff:

Status: Discarded