----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2987/#review6960 -----------------------------------------------------------
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? src/mem/ruby/network/MessageBuffer.hh (line 129) <http://reviews.gem5.org/r/2987/#comment5980> 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. src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc (line 117) <http://reviews.gem5.org/r/2987/#comment5981> 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. - Brad Beckmann On July 24, 2015, 4:45 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2987/ > ----------------------------------------------------------- > > (Updated July 24, 2015, 4:45 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10948:12a8ecefe478 > --------------------------- > 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 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. > > > Diffs > ----- > > src/mem/ruby/network/simple/PerfectSwitch.cc 9689ead7b479 > src/mem/ruby/network/simple/Throttle.hh 9689ead7b479 > src/mem/ruby/network/simple/Throttle.cc 9689ead7b479 > src/mem/ruby/slicc_interface/AbstractController.hh 9689ead7b479 > src/mem/ruby/slicc_interface/AbstractController.cc 9689ead7b479 > src/mem/ruby/slicc_interface/Message.hh 9689ead7b479 > src/mem/ruby/structures/RubyMemoryControl.hh 9689ead7b479 > src/mem/ruby/structures/RubyMemoryControl.cc 9689ead7b479 > src/mem/slicc/ast/FuncCallExprAST.py 9689ead7b479 > src/mem/slicc/ast/IfStatementAST.py 9689ead7b479 > src/mem/slicc/ast/InPortDeclAST.py 9689ead7b479 > src/mem/slicc/symbols/StateMachine.py 9689ead7b479 > src/mem/ruby/common/Consumer.hh 9689ead7b479 > src/mem/ruby/common/Consumer.cc 9689ead7b479 > src/mem/ruby/network/MessageBuffer.hh 9689ead7b479 > src/mem/ruby/network/MessageBuffer.cc 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 9689ead7b479 > src/mem/ruby/network/simple/PerfectSwitch.hh 9689ead7b479 > > Diff: http://reviews.gem5.org/r/2987/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
