> On May 22, 2013, 3:19 p.m., Alan Conway wrote: > > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.cpp, line 422 > > <https://reviews.apache.org/r/11329/diff/1/?file=295349#file295349line422> > > > > There is a lot of gritty encoding detail in Message for handling > > annotations. I suggest adding a MessageAnnotations class to hold this kind > > of detail to keep Message clean. > > Gordon Sim wrote: > Note that this Message class is for the AMQP 1.0 encoding (i.e. it is > different from qpid::broker::Message). Does that change your opinion?
Yes, that makes more sense. I still think that MessageAnnotations deserves to be a class in its own right - handling the 2 maps as one and doing the encoding is complex enough to be encapsulated. Is your optimization 1.0 only? Are your before/after numbers for 1.0 or 0.10? - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11329/#review20896 ----------------------------------------------------------- On May 22, 2013, 1:58 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11329/ > ----------------------------------------------------------- > > (Updated May 22, 2013, 1:58 p.m.) > > > Review request for qpid, Andrew Stitcher, Alan Conway, Kim van der Riet, and > Ernie Allen. > > > Description > ------- > > This attempts to reduce the performance impact of adding annotations (headers > in 0-10) to a message as it passes through the broker. At present this uses a > copy-on-write approaches that clones the header body, modifies it and then > uses that for encode or for writing to the wire. This change avoids the full > clone, by encoding the original header and while doing so adjusting for any > additions. > > Before: > > $ qpid-cpp-benchmark --repeat 5 --create-option > "node:{x-declare:{arguments:{'qpid.queue_msg_sequence':1}}}" > send-tp recv-tp l-min l-max l-avg total-tp > 30720 30671 0.32 44.03 18.28 30064 > 31778 31729 0.22 54.36 16.77 31068 > 31664 31637 0.39 69.50 22.11 31014 > 31303 31301 0.33 43.97 17.32 30737 > 31577 31403 0.33 37.73 15.59 30785 > > $ qpid-cpp-benchmark --repeat 5 > send-tp recv-tp l-min l-max l-avg total-tp > 47138 47120 0.13 29.02 9.35 45870 > 46299 46156 0.13 25.24 8.33 44803 > 45910 45793 0.13 53.53 10.81 45300 > 46607 46388 0.16 29.70 9.21 45245 > 46905 46766 0.16 31.43 9.62 45776 > > After: > > $ qpid-cpp-benchmark --repeat 5 --create-option > "node:{x-declare:{arguments:{'qpid.queue_msg_sequence':'seqno'}}}" > send-tp recv-tp l-min l-max l-avg total-tp > 44418 44376 0.15 32.29 10.24 43191 > 44127 44045 0.14 32.39 11.71 42762 > 44115 43882 0.20 37.27 13.42 42633 > 43444 43431 0.15 36.99 12.00 42147 > 44573 44265 0.17 34.87 11.81 42898 > > $ qpid-cpp-benchmark --repeat 5 > send-tp recv-tp l-min l-max l-avg total-tp > 47178 47061 0.16 27.23 8.46 45644 > 46291 46291 0.14 25.92 9.51 44812 > 47704 47525 0.17 32.19 9.06 45998 > 47771 47745 0.17 21.34 8.60 46271 > 43950 43931 0.13 52.13 10.60 43118 > > So, there is still some impact, but I believe it has been reduced. There are > some further things to try (caching encoded header as originally received to > avoid re-encoding it). > > > Diffs > ----- > > /trunk/qpid/cpp/src/CMakeLists.txt 1485001 > /trunk/qpid/cpp/src/Makefile.am 1485001 > /trunk/qpid/cpp/src/qpid/broker/Message.h 1485001 > /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1485001 > /trunk/qpid/cpp/src/qpid/broker/PersistableMessage.h 1485001 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1485001 > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1485001 > /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1485001 > /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1485001 > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1485001 > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.cpp 1485001 > /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1485001 > /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp 1485001 > /trunk/qpid/cpp/src/qpid/framing/AMQFrame.h 1485001 > /trunk/qpid/cpp/src/qpid/framing/AMQFrame.cpp 1485001 > /trunk/qpid/cpp/src/qpid/framing/FrameSet.h 1485001 > /trunk/qpid/cpp/src/qpid/framing/RawFrameBody.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/framing/RawFrameBody.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/11329/diff/ > > > Testing > ------- > > make check/test passes both on autotools and cmake builds > > > Thanks, > > Gordon Sim > >
