----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11329/#review20896 -----------------------------------------------------------
It probably makes little difference but why did you use qpid.queue_msg_sequence':1 before and qpid.queue_msg_sequence':'seqno' after in your before-after comparison? I think MessageAnnotations should be a class in its own right to encapsulate the encoding logic. There is duplication of framing and encoding logic, could existing encoding logic be refactored rather than duplicated? /trunk/qpid/cpp/src/qpid/broker/Message.h <https://reviews.apache.org/r/11329/#comment43024> typo: buy -> by /trunk/qpid/cpp/src/qpid/broker/Message.h <https://reviews.apache.org/r/11329/#comment43025> Manual step is error prone, can you do it automatically when the persistent context is requested? /trunk/qpid/cpp/src/qpid/broker/Message.h <https://reviews.apache.org/r/11329/#comment43026> Presumably you split the annotations map into string/int because Variant overhead was a performance problem. Should comment that here so somebody doesn't later go "hey, I could just use a VariantMap here" /trunk/qpid/cpp/src/qpid/broker/Message.cpp <https://reviews.apache.org/r/11329/#comment43027> What's a CharSequence? /trunk/qpid/cpp/src/qpid/broker/amqp/Message.cpp <https://reviews.apache.org/r/11329/#comment43029> This is a lot of repetitive code, but I'm not sure how to do it more neatly. /trunk/qpid/cpp/src/qpid/broker/amqp/Message.cpp <https://reviews.apache.org/r/11329/#comment43032> 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. /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp <https://reviews.apache.org/r/11329/#comment43033> Could this be encapsulated in a MessageAnnotations class? /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp <https://reviews.apache.org/r/11329/#comment43034> Why do we have to create all this encoding detail? Don't we already have coded that encodes properties etc? - Alan Conway 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 > >
