> On May 19, 2014, 3:05 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 86 > > <https://reviews.apache.org/r/21483/diff/1/?file=582523#file582523line86> > > > > This doesn't seem right to me: > > > > The previous Encoding was an interface (pure abstract class) and this > > change has given it state. > > > > I think maybe you should add extra member functions to the interface > > but use the SharedState implementation class as a mixin to the actual leaf > > implementation classes rather than mixing in to this interface.
Encoding already had some inhertoed state, however I have changed SharedState to be an interface also, and separated the implementation class out. > On May 19, 2014, 3:05 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 171 > > <https://reviews.apache.org/r/21483/diff/1/?file=582523#file582523line171> > > > > I don't think this Optional has much advantage over just using a simple > > boost::scoped_ptr. The only difference is explicit vs implicit copying. > > > > If you really want to keep it using a boost::scoped_ptr in there would > > reduce it to just a few lines. > > Not sure what you mean in the first paragraph. boost::scoped_ptr is noncopyable and I'd rather not have to define the assignment and copy constructor for Message itself. Doing so in a wrapper around the optional member seemed neater and makes it easy to do the same if any similar state is ever added. I have however changed it to use boost::scoped_ptr - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21483/#review43362 ----------------------------------------------------------- On May 20, 2014, 11:46 a.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21483/ > ----------------------------------------------------------- > > (Updated May 20, 2014, 11:46 a.m.) > > > Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec. > > > Repository: qpid > > > Description > ------- > > Previous releases (e.g. 0.18) used to shared almost all message state. This > was not correct and the current codebase only shares the incoming message as > read off the wire. However, this leads to higher memory consumption in the > fanout case. > > There are some optimisations we could make to that. Two different changes are > included in this patch for review and comment. With these both in place, 1000 > messages shared between 500 queues dropped from taking around 81k to taking > around 43k as compared with the 0.18 release which took around 27k. > > The two changes are: > > (1) share some of the immutable items that don't arrive over the wire but > don't need to be changed per queue, e.g. expiration and publisher > > (2) make the annotations map (which seems to take a large amount of memory > even when empty) optional using a custom template (the boost optional > template doesn't save any memory). > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Message.h 1594633 > /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1594633 > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1594633 > /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1594633 > /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1594633 > /trunk/qpid/cpp/src/tests/MessageUtils.h 1594633 > /trunk/qpid/cpp/src/tests/QueueTest.cpp 1594633 > > Diff: https://reviews.apache.org/r/21483/diff/ > > > Testing > ------- > > make test passes > > > Thanks, > > Gordon Sim > >
