You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2014/05/07 12:00:36 UTC
Review Request 21153: [C++ broker] Make Queue::purgeExpired more efficient
by calling AbsTime::now() just once
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/
-----------------------------------------------------------
Review request for qpid, Gordon Sim and mick goulish.
Bugs: QPID-5748
https://issues.apache.org/jira/browse/QPID-5748
Repository: qpid
Description
-------
Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
The patch is not ideal: I would rather see just one function like:
bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
{
if time==0 time=sys::AbsTime::now();
return m.getExpiration() < sys::AbsTime::now();
}
but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1592542
/trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1592542
/trunk/qpid/cpp/src/qpid/broker/Message.h 1592542
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1592542
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1592542
Diff: https://reviews.apache.org/r/21153/diff/
Testing
-------
Attached patch improved:
- _traversing_ queues and _checking_ messages with TTL by approx. 10%
- with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
No automated tests run so far (lack of time+HW).
Thanks,
Pavel Moravec
Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more
efficient by calling AbsTime::now() just once
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/#review42675
-----------------------------------------------------------
Looks good to me. Might be worth a comment in the new ExpiryPolicy method, explaining why the time is passed in. I'm not sure what function the expiry policy serves anymore anyway. It used to be used in the old cluster to eliminate a source of indeterminacy. Not sure if it is needed anymore... (without it, could simply have the message return its expiration and do comparison against any time suitable).
- Gordon Sim
On May 7, 2014, 10 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21153/
> -----------------------------------------------------------
>
> (Updated May 7, 2014, 10 a.m.)
>
>
> Review request for qpid, Gordon Sim and mick goulish.
>
>
> Bugs: QPID-5748
> https://issues.apache.org/jira/browse/QPID-5748
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
>
> That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
>
> The patch is not ideal: I would rather see just one function like:
>
> bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
> {
> if time==0 time=sys::AbsTime::now();
> return m.getExpiration() < sys::AbsTime::now();
> }
>
> but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1592542
> /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1592542
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1592542
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1592542
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1592542
>
> Diff: https://reviews.apache.org/r/21153/diff/
>
>
> Testing
> -------
>
> Attached patch improved:
> - _traversing_ queues and _checking_ messages with TTL by approx. 10%
> - with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
>
> No automated tests run so far (lack of time+HW).
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more
efficient by calling AbsTime::now() just once
Posted by Gordon Sim <gs...@redhat.com>.
> On May 27, 2014, 8:40 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 117
> > <https://reviews.apache.org/r/21153/diff/2/?file=594322#file594322line117>
> >
> > Rather than have a hasExpired() method that takes the tie to compare against, it might be simpler to remove this method entirely and just use getExpiration() which can then be compared against whatever value is desired.
>
> Pavel Moravec wrote:
> hasExpired(Message, AbsTime) is required by Queue::purgeExpired:
>
> uint32_t count = remove(0, boost::bind(&Message::hasExpired, _1, time), 0, CONSUMER, settings.autodelete);
>
> Or is there a way how to boost::bind getExpiration() method there?
>
> The implicit setting time = sys::AbsTime::now() can be removed as that is used only in Queue::getNextMessage, but how to deal with the bind?
The second argument to remove() is any function/functor. So e.g. you could define
namespace{
bool hasExpired(const Message* m, AbsTime now)
{
return m->getExpiration() < now;
}
}
and then change the call to:
uint32_t count = remove(0, boost::bind(&hasExpired, _1, time), 0, CONSUMER, settings.autodelete);
The advantage here is that it isn't part of the Message interface, and is defined closer to where it is actually used.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/#review43955
-----------------------------------------------------------
On May 26, 2014, 12:14 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21153/
> -----------------------------------------------------------
>
> (Updated May 26, 2014, 12:14 p.m.)
>
>
> Review request for qpid, Gordon Sim and mick goulish.
>
>
> Bugs: QPID-5748
> https://issues.apache.org/jira/browse/QPID-5748
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
>
> That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
>
> The patch is not ideal: I would rather see just one function like:
>
> bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
> {
> if time==0 time=sys::AbsTime::now();
> return m.getExpiration() < sys::AbsTime::now();
> }
>
> but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessage.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessageImpl.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1597555
> /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1597555
> /trunk/qpid/cpp/src/tests/MessageUtils.h 1597555
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1597555
>
> Diff: https://reviews.apache.org/r/21153/diff/
>
>
> Testing
> -------
>
> Attached patch improved:
> - _traversing_ queues and _checking_ messages with TTL by approx. 10%
> - with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
>
> No automated tests run so far (lack of time+HW).
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more
efficient by calling AbsTime::now() just once
Posted by Pavel Moravec <pm...@redhat.com>.
> On May 27, 2014, 8:40 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Message.h, line 117
> > <https://reviews.apache.org/r/21153/diff/2/?file=594322#file594322line117>
> >
> > Rather than have a hasExpired() method that takes the tie to compare against, it might be simpler to remove this method entirely and just use getExpiration() which can then be compared against whatever value is desired.
hasExpired(Message, AbsTime) is required by Queue::purgeExpired:
uint32_t count = remove(0, boost::bind(&Message::hasExpired, _1, time), 0, CONSUMER, settings.autodelete);
Or is there a way how to boost::bind getExpiration() method there?
The implicit setting time = sys::AbsTime::now() can be removed as that is used only in Queue::getNextMessage, but how to deal with the bind?
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/#review43955
-----------------------------------------------------------
On May 26, 2014, 12:14 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21153/
> -----------------------------------------------------------
>
> (Updated May 26, 2014, 12:14 p.m.)
>
>
> Review request for qpid, Gordon Sim and mick goulish.
>
>
> Bugs: QPID-5748
> https://issues.apache.org/jira/browse/QPID-5748
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
>
> That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
>
> The patch is not ideal: I would rather see just one function like:
>
> bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
> {
> if time==0 time=sys::AbsTime::now();
> return m.getExpiration() < sys::AbsTime::now();
> }
>
> but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessage.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessageImpl.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1597555
> /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1597555
> /trunk/qpid/cpp/src/tests/MessageUtils.h 1597555
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1597555
>
> Diff: https://reviews.apache.org/r/21153/diff/
>
>
> Testing
> -------
>
> Attached patch improved:
> - _traversing_ queues and _checking_ messages with TTL by approx. 10%
> - with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
>
> No automated tests run so far (lack of time+HW).
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more
efficient by calling AbsTime::now() just once
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/#review43955
-----------------------------------------------------------
Looks good to me! Cleans things up nicely.
/trunk/qpid/cpp/src/qpid/broker/Message.h
<https://reviews.apache.org/r/21153/#comment78215>
Rather than have a hasExpired() method that takes the tie to compare against, it might be simpler to remove this method entirely and just use getExpiration() which can then be compared against whatever value is desired.
- Gordon Sim
On May 26, 2014, 12:14 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21153/
> -----------------------------------------------------------
>
> (Updated May 26, 2014, 12:14 p.m.)
>
>
> Review request for qpid, Gordon Sim and mick goulish.
>
>
> Bugs: QPID-5748
> https://issues.apache.org/jira/browse/QPID-5748
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
>
> That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
>
> The patch is not ideal: I would rather see just one function like:
>
> bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
> {
> if time==0 time=sys::AbsTime::now();
> return m.getExpiration() < sys::AbsTime::now();
> }
>
> but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/CMakeLists.txt 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessage.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoverableMessageImpl.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1597555
> /trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1597555
> /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1597555
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1597555
> /trunk/qpid/cpp/src/tests/MessageUtils.h 1597555
> /trunk/qpid/cpp/src/tests/QueueTest.cpp 1597555
>
> Diff: https://reviews.apache.org/r/21153/diff/
>
>
> Testing
> -------
>
> Attached patch improved:
> - _traversing_ queues and _checking_ messages with TTL by approx. 10%
> - with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
>
> No automated tests run so far (lack of time+HW).
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 21153: [C++ broker] Make Queue::purgeExpired more
efficient by calling AbsTime::now() just once
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21153/
-----------------------------------------------------------
(Updated May 26, 2014, 12:14 p.m.)
Review request for qpid, Gordon Sim and mick goulish.
Changes
-------
Updated patch proposal ("svn diff" lacks removal of qpid/broker/ExpiryPolicy.h and qpid/broker/ExpiryPolicy.cpp files).
Based on Gordon's hint to remove ExpiryPolicy from Message class, it turns out ExpiryPolicy is not required anywhere.
Bugs: QPID-5748
https://issues.apache.org/jira/browse/QPID-5748
Repository: qpid
Description
-------
Queue::purgeExpired method (to remove messages from all queues with TTL expired) currently calls Message::hasExpired() method that calls AbsTime::now() i.e. ::clock_gettime for every individual message with TTL set.
That system call is redundant to be called in that way. It is enough to call it once before traversing the very first queue and use the value as an argument.
The patch is not ideal: I would rather see just one function like:
bool ExpiryPolicy::hasExpired(const Message& m, qpid::sys::AbsTime time = 0)
{
if time==0 time=sys::AbsTime::now();
return m.getExpiration() < sys::AbsTime::now();
}
but not sure what value from AbsTime to be used instead of 0 (note that such value should be get in as short time as possible). And I havent played with AbsTime class and dont have time now to do so :-/
Diffs (updated)
-----
/trunk/qpid/cpp/src/CMakeLists.txt 1597555
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1597555
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/Message.h 1597555
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1597555
/trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/RecoverableMessage.h 1597555
/trunk/qpid/cpp/src/qpid/broker/RecoverableMessageImpl.h 1597555
/trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1597555
/trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1597555
/trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1597555
/trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1597555
/trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1597555
/trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1597555
/trunk/qpid/cpp/src/tests/MessageUtils.h 1597555
/trunk/qpid/cpp/src/tests/QueueTest.cpp 1597555
Diff: https://reviews.apache.org/r/21153/diff/
Testing
-------
Attached patch improved:
- _traversing_ queues and _checking_ messages with TTL by approx. 10%
- with purging _durable_ expired messages, performance improvement is just 1% (still better than nothing)
No automated tests run so far (lack of time+HW).
Thanks,
Pavel Moravec