You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2013/05/22 15:58:40 UTC
Review Request: Performance improvements to Message::addAnnotations()
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11329/
-----------------------------------------------------------
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
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Alan Conway <ac...@redhat.com>.
> 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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Gordon Sim <gs...@redhat.com>.
> On May 22, 2013, 3:19 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 109
> > <https://reviews.apache.org/r/11329/diff/1/?file=295341#file295341line109>
> >
> > Manual step is error prone, can you do it automatically when the persistent context is requested?
The reason I didn't do that is that requesting the persistent context is currently defined as a const method. If I do actually modify the internal state of the message then I need to explicitly make the relevant members mutable and that complicates the concurrency picture a little. I'm not delighted with the explicit pack however...
> On May 22, 2013, 3:19 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 158
> > <https://reviews.apache.org/r/11329/diff/1/?file=295341#file295341line158>
> >
> > 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"
Yes indeed that was the reason and I will add a comment.
> On May 22, 2013, 3:19 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.cpp, line 231
> > <https://reviews.apache.org/r/11329/diff/1/?file=295342#file295342line231>
> >
> > What's a CharSequence?
It avoids converting data into a string, i.e. it allows you to identify particular parts of a larger block of memory to avoid unpacking it and building up some other structure. All it is is a pointer and a limit.
> 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.
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?
> On May 22, 2013, 3:19 p.m., Alan Conway 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>
> >
> > Could this be encapsulated in a MessageAnnotations class?
I guess I could split it out, yes. (There would need to be two different annotations classes for 0-10 and 1.0 - currently the code is in the classes encapsulating the encoding of those formats).
> On May 22, 2013, 3:19 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp, line 246
> > <https://reviews.apache.org/r/11329/diff/1/?file=295351#file295351line246>
> >
> > Why do we have to create all this encoding detail? Don't we already have coded that encodes properties etc?
Yes, but the problem is that it is very tied to complete structures and what I want to avoid is copying and modifying those structures.
- Gordon
-----------------------------------------------------------
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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Gordon Sim <gs...@redhat.com>.
> 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?
>
> Alan Conway wrote:
> 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?
No the optimisation covers 0-10 as well, which is what I used for the numbers reported. I'll separate out the code.
- Gordon
-----------------------------------------------------------
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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Andrew Stitcher <as...@apache.org>.
> 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.
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.
- Andrew
-----------------------------------------------------------
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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request: Performance improvements to Message::addAnnotations()
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11329/#review20897
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/11329/#comment43030>
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>.
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/11329/#comment43028>
sp. buy -> by
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/11329/#comment43031>
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.
/trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp
<https://reviews.apache.org/r/11329/#comment43041>
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.
- Andrew Stitcher
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
>
>