[jbossws-dev] Fwd: EndpointMetrics Refactor

[Alessio Soldano]

Hi Jim,

On 09/07/14 12:16, Jim Ma wrote:
> 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?
To me yes, we should, I believe it's worth the additional readability 
cost. But we can do this as a separate task in the next future, if you 
prefer.

>> * 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.
Good, I see you already fixed this.


>> * 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.
As above, fixed, good.

>>
>>  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.
Sure, for atomic changes. Moreover, since you're setting all the 
attributes with default starting values, add a 'final' to all of them to 
explicitly ensure visibility.

>> * 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.
OK

>> * 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.
Right, as commented above.

>> * 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 ?
For sure any lock can add some delay; the idea is that the R/W lock 
mechanism could basically prevent threads recording the response time 
from blocking, unless someone is getting the average response time at 
the same time (as the access to the average would use the W lock). Some 
profiling / perf test could confirm the actual impact.
Compare and set (CAS) is of no use here, imho, because you need the 
result of 2 independent method invocations (retrieval of 
totalHandlingTime value and invocation number value).

>>
>>  EndpointAssociationInterceptor
>> -------------------------------------------
>> * We should probably set the service counter name in the exchange 
>> only when the statistics are enabled.
>  I'll refactor this.
OK, thanks

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

Cheers
Alessio

-- 
Alessio Soldano
Web Service Lead, JBoss

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