Re: Review Request 23125: reduction of throughput regression

[Gordon Sim]

---
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


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


As above I think we should leave this in.



trunk/qpid/cpp/src/qpid/broker/amqp_0_10/MessageTransfer.h


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


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


I would just assert on cachedRequiredCredit (and maybe rename it to 
haveComputedRequiredCredit).



trunk/qpid/cpp/src/qpid/framing/FrameSet.h


Minor nit: spurious whitespace.



trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp


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
> 
>

Review Request 23125: reduction of throughput regression

[mick goulish]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23125/
---

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