> 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. > > Gordon Sim wrote: > 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. > > Andrew Stitcher wrote: > Yes you are correct about the duplicated code in Codecs, probably that > code could use this as well. > > Although it may be a legacy concern, all this extra duplication makes it > continually harder to understand the code base and hence maintain it. So > unless we're not going to support 0.10 in the near future in which case we > can remove the code and be done with it, we will need to maintain the code. > > But of course, it does always come down to ROI at some level.
Exactly, its a question of ROI. Though the same function is being done in three different places (Codecs, here and FieldTable) they all have different source structures. In Codecs it is a Variant::Map, in FieldTable it is the map of FieldValue pointers and here it is a more abstracted source that hides how the actual values are stored (also as above, here we split the encoding of values in the map from the encoding of the map as a whole. Could Codecs and FieldTable be modified to pump their structures through a MapHandler (which would be relocated to the common package) and thus share the small amount of common code (put type code, put typed value)? yes, of course. That wouldn't improve the readability/maintainability of FieldTable however. It would still be confusing and annoying to have the different apopraches. The only solution that IMHO gives tangible maintenance benefits would be wholesale removal of FieldTable from (at least) the broker codebase. That is something I would dearly love to do. It isn't a small change though. As and when I have time I'll return to this problem and see whether by altering the brokers handling of 0-10 header frames more radically I can make a significant simpification overall (as well as possibly some further performance gains). - 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 > >
