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 2014/05/15 13:45:07 UTC
Review Request 21483: Improvements to per-message memory overhead in fanout
case
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/
-----------------------------------------------------------
Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
Repository: qpid
Description
-------
Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
The two changes are:
(1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
(2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
Diff: https://reviews.apache.org/r/21483/diff/
Testing
-------
make test passes
Thanks,
Gordon Sim
Re: Review Request 21483: Improvements to per-message memory overhead in
fanout case
Posted by Gordon Sim <gs...@redhat.com>.
> On May 19, 2014, 3:05 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 86
> > <https://reviews.apache.org/r/21483/diff/1/?file=582523#file582523line86>
> >
> > This doesn't seem right to me:
> >
> > The previous Encoding was an interface (pure abstract class) and this change has given it state.
> >
> > I think maybe you should add extra member functions to the interface but use the SharedState implementation class as a mixin to the actual leaf implementation classes rather than mixing in to this interface.
Encoding already had some inhertoed state, however I have changed SharedState to be an interface also, and separated the implementation class out.
> On May 19, 2014, 3:05 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 171
> > <https://reviews.apache.org/r/21483/diff/1/?file=582523#file582523line171>
> >
> > I don't think this Optional has much advantage over just using a simple boost::scoped_ptr. The only difference is explicit vs implicit copying.
> >
> > If you really want to keep it using a boost::scoped_ptr in there would reduce it to just a few lines.
> >
Not sure what you mean in the first paragraph. boost::scoped_ptr is noncopyable and I'd rather not have to define the assignment and copy constructor for Message itself. Doing so in a wrapper around the optional member seemed neater and makes it easy to do the same if any similar state is ever added.
I have however changed it to use boost::scoped_ptr
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/#review43362
-----------------------------------------------------------
On May 20, 2014, 11:46 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21483/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 11:46 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
>
> There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
>
> The two changes are:
>
> (1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
>
> (2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1594633
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1594633
> /trunk/qpid/cpp/src/tests/MessageUtils.h 1594633
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1594633
>
> Diff: https://reviews.apache.org/r/21483/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21483: Improvements to per-message memory overhead in
fanout case
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/#review43362
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/21483/#comment77448>
This doesn't seem right to me:
The previous Encoding was an interface (pure abstract class) and this change has given it state.
I think maybe you should add extra member functions to the interface but use the SharedState implementation class as a mixin to the actual leaf implementation classes rather than mixing in to this interface.
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/21483/#comment77449>
I don't think this Optional has much advantage over just using a simple boost::scoped_ptr. The only difference is explicit vs implicit copying.
If you really want to keep it using a boost::scoped_ptr in there would reduce it to just a few lines.
- Andrew Stitcher
On May 15, 2014, 11:45 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21483/
> -----------------------------------------------------------
>
> (Updated May 15, 2014, 11:45 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
>
> There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
>
> The two changes are:
>
> (1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
>
> (2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
>
> Diff: https://reviews.apache.org/r/21483/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21483: Improvements to per-message memory overhead in
fanout case
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/#review43477
-----------------------------------------------------------
Ship it!
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/21483/#comment77640>
Some easy things to try, might shave a few bytes off:
- Sort Message members by alignment, 64 bit integers first, then pointers, then 32 ints, etc. Maybe the compiler will pack them better.
- Put these last and use bit fields:
MessageState state:4;
bool alreadyAcquired:1;
/trunk/qpid/cpp/src/qpid/broker/Message.cpp
<https://reviews.apache.org/r/21483/#comment77635>
I think we can get rid of expirationPolicy, it was only used by old cluster.
- Alan Conway
On May 20, 2014, 11:46 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21483/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 11:46 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
>
> There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
>
> The two changes are:
>
> (1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
>
> (2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1594633
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1594633
> /trunk/qpid/cpp/src/tests/MessageUtils.h 1594633
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1594633
>
> Diff: https://reviews.apache.org/r/21483/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21483: Improvements to per-message memory overhead in
fanout case
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/
-----------------------------------------------------------
(Updated May 20, 2014, 11:46 a.m.)
Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
Changes
-------
Made SharedState a pure abstract class, with SharedStateImpl as an implementation of that. SharedState now extends Encoding, rather than the other way round (which was 'convenient' but logically incorrect). Modified Optional template to use scoped_ptr. Removed methods from Message that modify shared state, requiring the shared state to be obtained explicitly to make the impact clearer (there are only three contexts that require access to this - would eventually be nice to avoid the need altogether).
Repository: qpid
Description
-------
Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
The two changes are:
(1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
(2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1594633
/trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1594633
/trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1594633
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1594633
/trunk/qpid/cpp/src/tests/MessageUtils.h 1594633
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1594633
Diff: https://reviews.apache.org/r/21483/diff/
Testing
-------
make test passes
Thanks,
Gordon Sim
Re: Review Request 21483: Improvements to per-message memory overhead in
fanout case
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21483/#review43204
-----------------------------------------------------------
Ship it!
Ship It!
- Pavel Moravec
On May 15, 2014, 11:45 a.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21483/
> -----------------------------------------------------------
>
> (Updated May 15, 2014, 11:45 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, and Pavel Moravec.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Previous releases (e.g. 0.18) used to shared almost all message state. This was not correct and the current codebase only shares the incoming message as read off the wire. However, this leads to higher memory consumption in the fanout case.
>
> There are some optimisations we could make to that. Two different changes are included in this patch for review and comment. With these both in place, 1000 messages shared between 500 queues dropped from taking around 81k to taking around 43k as compared with the 0.18 release which took around 27k.
>
> The two changes are:
>
> (1) share some of the immutable items that don't arrive over the wire but don't need to be changed per queue, e.g. expiration and publisher
>
> (2) make the annotations map (which seems to take a large amount of memory even when empty) optional using a custom template (the boost optional template doesn't save any memory).
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1594633
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1594633
>
> Diff: https://reviews.apache.org/r/21483/diff/
>
>
> Testing
> -------
>
> make test passes
>
>
> Thanks,
>
> Gordon Sim
>
>