> On July 24, 2015, 4:48 p.m., Jason Power wrote: > > I'm not sure how I feel about this patch (and the other ones posted today) > > in general, but if we are going to move away from reference counting, then > > this patch needs to include very specific information on which objects are > > responsible for deleting any heap variables. As it stands, it would be very > > hard for someone to modify this code without introducing spurious memory > > bugs. > > Jason Power wrote: > Nilay said: > > The question of responsibility arises when multiple objects may hold > > pointer to a heap allocated object. As per my understanding, this is > not > > the case for any of the protocols and on-chip network implementations. > > So the current owner can choose to delete the message as soon as it > > realizes that the message is no longer required. > > So you're saying that for it is always the receiver's responsibility to > delete the message? Is this the same protocol we use for packets? I just > worry about memory bugs creeping up when someone is modifying a protocol. > That would be incredibly hard to track down!
The explicit allocation/deletion in the classic memory system is a tricky story, and we have looked at using shared_ptr quite a few times. Only doing it for requests costs us around 10% performance. For packets it's worse. The reason the new/delete is not straight forward is that packets have different lifetimes. Request packets that do not need responses are deleted when they reach the slave, e.g. a memory. Packets that need need a response are typically re-used, and the response packet is deleted at the master, e.g. a device or CPU. The big issue is forwarding of packets, which happens in the caches. Here it gets rather tricky to track the lifetime of the request (outliving the copy), and each packet. If changing to explicit allocation/deletion brings a lot of performance, then perhaps it is the right answer. Beware though...the devil is in the details. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2989/#review6824 ----------------------------------------------------------- On July 24, 2015, 4:46 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2989/ > ----------------------------------------------------------- > > (Updated July 24, 2015, 4:46 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10952:57fd09945e80 > --------------------------- > ruby: switch to explicit allocation and deletion of messages > Currently messages in ruby are reference counted objects. This is a > performance issue > since c++ reference counting uses atomic instructions for maintaining the > counts. > This patch explicitly allocates and deletes messages. We see a performance > improvement > of about 3-5% on the only ruby fs regression test. > > > Diffs > ----- > > src/mem/ruby/slicc_interface/Message.cc PRE-CREATION > src/mem/ruby/slicc_interface/RubyRequest.hh 9689ead7b479 > src/mem/ruby/slicc_interface/SConscript 9689ead7b479 > src/mem/ruby/structures/WireBuffer.hh 9689ead7b479 > src/mem/ruby/structures/WireBuffer.cc 9689ead7b479 > src/mem/ruby/system/DMASequencer.cc 9689ead7b479 > src/mem/ruby/system/Sequencer.cc 9689ead7b479 > src/mem/slicc/symbols/Type.py 9689ead7b479 > src/mem/protocol/MESI_Two_Level-L1cache.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-L2cache.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-dir.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-dma.sm 9689ead7b479 > src/mem/ruby/network/MessageBuffer.hh 9689ead7b479 > src/mem/ruby/network/MessageBuffer.cc 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.hh > 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/RoutingUnit_d.cc 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/flit_d.hh 9689ead7b479 > src/mem/ruby/network/garnet/fixed-pipeline/flit_d.cc 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.hh > 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/flit.hh 9689ead7b479 > src/mem/ruby/network/garnet/flexible-pipeline/flit.cc 9689ead7b479 > src/mem/ruby/network/simple/PerfectSwitch.cc 9689ead7b479 > src/mem/ruby/network/simple/Throttle.cc 9689ead7b479 > src/mem/ruby/slicc_interface/AbstractController.cc 9689ead7b479 > src/mem/ruby/slicc_interface/Message.hh 9689ead7b479 > > Diff: http://reviews.gem5.org/r/2989/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
