---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23125/#review46848
---
trunk/qpid/cpp/src/qpid/broker/MessageBuilder.cpp
https://reviews.apache.org/r/23125/#comment82414
I prefer keeping a clear distinction between computing and getting the
required credit. What I *would* do is remove the auto-computation inside
getRequiredCredit(). I would treat get without compute as an error.
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h
https://reviews.apache.org/r/23125/#comment82415
As above I think we should leave this in.
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h
https://reviews.apache.org/r/23125/#comment82417
These shouldn't be mutable. The reason I want to keep compute and get
separate is that one should be genuinely const (i.e. make no changes and thus
be safe to call from different threads) and the other should explicitly be
about changing state and should be done in a context where no other thread has
access to the object being changed.
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h
https://reviews.apache.org/r/23125/#comment82416
Irrelevant if we leave this method as is, but it makes no sense to define
it as const, since its whole purpose is to alter state.
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp
https://reviews.apache.org/r/23125/#comment82418
I would just assert on cachedRequiredCredit (and maybe rename it to
haveComputedRequiredCredit).
trunk/qpid/cpp/src/qpid/framing/FrameSet.h
https://reviews.apache.org/r/23125/#comment82419
Minor nit: spurious whitespace.
trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp
https://reviews.apache.org/r/23125/#comment82420
Again, this should be compute, not get.
- Gordon Sim
On June 27, 2014, 10:44 a.m., mick goulish wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23125/
---
(Updated June 27, 2014, 10:44 a.m.)
Review request for qpid, Alan Conway and Gordon Sim.
Repository: qpid
Description
---
This is the diff for trunk - not previous versions.
This diff is *not* quite the same as the recent diffs I have sent you (Gordon
and Alan). It has one additional change.
Code outside of MessageTransfer no longer needs to worry about when to call
getRequiredCredit or computeRequiredCredit.Outside code always calls
getRequiredCredit. That fn calls computeRequiredCredit and caches the
result, thereafter using only the cached result.
This eliminates problems where get() was being called before compute() -- but
I felt that was a problem that code outside of MessageTransfer should not
have to worry about.
I was not able to remove the const-ness of getRequiredCredit without causing
a vast cataclysm of deconstification, so I left it const, but made mutable
the two variables that it needs to change.
Change list for people who have not seen previous versions:
* replace frame.map_if with purpose-written loop. This reduced throughput
regression by about half.
* small change in SessionState to make sure requiredCredit gets calculated
before routing is done.
* all code outside of MessageTransfer now only calls getRequiredCredit. If
the number has not
yet been calculated -- that problem is encapsulated within
MessageTransfer.
Diffs
-
trunk/qpid/cpp/src/qpid/broker/MessageBuilder.cpp 1605215
trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605215
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h 1605215
trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.cpp 1605215
trunk/qpid/cpp/src/qpid/framing/FrameSet.h 1605215
trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1605215
Diff: https://reviews.apache.org/r/23125/diff/
Testing
---
make check, throughput testing (see graph)
Hey. how do I attach things?!? dang it. ok -- I will attach pretty graph
to BZ https://bugzilla.redhat.com/show_bug.cgi?id=1011221
Thanks,
mick goulish
