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