> On May 22, 2013, 3:35 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 31 > > <https://reviews.apache.org/r/11329/diff/1/?file=295341#file295341line31> > > > > Generally it'll be faster to use an unordered_map than a map (obviously > > no good if you actually need the sorted property of maps) - #include > > "qpid/sys/unordered_map.h" and use unordered_map<T> instead of map<T>.
Thanks, I'll try that out. > On May 22, 2013, 3:35 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 158 > > <https://reviews.apache.org/r/11329/diff/1/?file=295341#file295341line158> > > > > Does having 2 annotation stores give much of a speed up compared to > > just using 1 with a Variant? Which seems more natural (to me at least) > > > > If you are going to have a separate integer store possibly it should be > > of int64_t/uint64_t so it can hold any size int. I agree the Variant approach was 'nicer', the speed-up was observable however so I included it. I think the problem with Variant is that the PIMPL idiom which is really only needed on the client side results in extra allocations. A more efficient equivalent would also be possible. > On May 22, 2013, 3:35 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp, line 147 > > <https://reviews.apache.org/r/11329/diff/1/?file=295351#file295351line147> > > > > Much of the following code seems like it isn't specific to > > MessageTransfer but is rather another implementation of the FieldTable > > encoder, doing a very similar function (encoding a map<string,Variant>). > > The only real difference is the use of CharSequence instead of string. > > > > There must be a less duplicative solution, possibly making the > > FieldTable encoder use this functionality. > > > > In any case I'd advise putting this code in it's own file as something > > like qpid/framing/MapEncoder.(h|cpp) where it might be used elsewhere. Yes, a large part of this patch adds code that is very similar to existing encoding logic. The key difference (apart from the use of CharSequence) is that it does 'partial' encoding of structures which existing logic can only encode as a whole. This is the heart of the approach (i.e. don't clone, simply merge in changes while encoding). A this point I would consider 0-10 encoding logic something of a legacy concern. I'm personally not convinced updating the FieldTable encoding logic to include this is really justified (there is already code duplication between that and qpid::amqp_0_10::Codecs). A more complete solution would be to avoid using FieldTable entirely on the broker side. As always the question is where to draw the line in concrete improvement and ever deeper entanglement in the consequences of changes. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11329/#review20897 ----------------------------------------------------------- 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 > >
