[jbossws-dev] Fwd: EndpointMetrics Refactor

[Jim Ma]

Hi Alessio,

Thanks for the review! Please see my comments inline.

On 08/07/14 18:45, Alessio Soldano wrote:
>  ResponseTime(*)Interceptor
> --------------------------------------
> * The AbstractResponseTimeInterceptor#getServiceCounterName method 
> will be using the counter name we set in the 
> EndpointAssociationInterceptor, good. However, from a CXF point of 
> view, the name could be computed each time, so the code doing that 
> needs to be decently optimized. In particular, the StringBuilder 
> should be properly used there; if there's a StringBuilder being used, 
> you should really avoid string concatenations to add stuff in the 
> builder. The JDK optimizer will replace the string concatenations with 
> additional StringBuilder instances, which is a waste of resources. So 
> basically use multiple append() invocations instead of String 
> concatenations (unless the String constants only are being concatenated).
  These are all legacy codes from cxf, and another 5 StringBuilder 
instance will be created for the string concatenations. But it is more 
readable than all with append(). Should we pay the additional resources 
to get readability?
> * same reasoning for 
> AbstractResponseTimeInterceptor#getOperationCounterName, plus please 
> use StringBuilder instead of StringBuffer, as that's not accessed 
> concurrently.
   I wrote this wrong. It's a local variable and doesn't have some 
concurrent issue,so StringBuilder will be fine, no need StringBuff.
> * In all ResponseTimeMessage(*)Interceptor, the check on 
> "forceDisabled" should come before the isServiceCounterEnabled() 
> invocation, as that's expensive. So basically do "if !forceDisabled && 
> (isServiceCounterEnabled(ex))" instead of "if 
> (isServiceCounterEnabled(ex) && !forceDisabled)"
  I'll make this change.
>
>  ResponseTimeCounter
> -------------------------------
> * Generally speaking I have some concerns on the thread safety of this 
> class, in particular on visibility. Shouldn't we have final or 
> volatile attributes?
  Looked at this class again.  The totalHandlingTime, minHandlingTime 
and maxHandlingTime should be all atomic variable.
> * At line 87, did you really want to use "|" or "||" ?
  typo !
> * Is it possible to end up with handlingTime = 0 for oneway messages 
> and hence end up setting min handling time to 0 ? Is that reasonable ? 
> (probably no...)
  no. The oneway message will go into several interceptors and consume 
some time to finish the process.
> * the update of min and max handling time in not performed in a thread 
> safe way; please have a look at how that should possible be done in 
> our former EndpointMetricImpl (see updateMin / updateMax in [1])
  It should be all atomic variable here.
> * it is possible to get unreliable values for the average response 
> time in highly concurrent environments; we might decide to accept this 
> or solve as we did in our EndpointMetricsImpl (see the usage or 
> read/write lock and related comment in [1])
   I am not sure if the lock is a bit heavy here. It is possible to 
affect the interceptor execution performance ,  and it will wait the 
lock, then the last interceptor MessageSenderEndingInterceptor can 
really send the response to the client.Do you
   think compareAndSet()  can help solve this problem ?

>
>  EndpointAssociationInterceptor
> -------------------------------------------
> * We should probably set the service counter name in the exchange only 
> when the statistics are enabled.
  I'll refactor this.
>
>  EndpointMetricsCXFAdapterImpl
> --------------------------------------------
> * counterRepo and counter members should be final, while there's no 
> need for keeping a reference to the objectName
   Make it as method local final variable will be better.


Cheers,
Jim


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/jbossws-dev/attachments/20140709/40cd5cbe/attachment.html