qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request 23125: reduction of throughput regression
Date Fri, 27 Jun 2014 10:55:26 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message