ThreadLocal vs object pool

[Dmytro Rud]

Dear colleagues,

ThreadLocals are used at different places in IPF -- in the most cases, the only purpose of it is to hold non thread-safe objects like DOM builders.  But the threads, from where the ThreadLocals are used, are per se not pooled, therefore the objects stored in TheradLocals are effectively not reused -- every call to get() results in a call to initialValue().

My proposal is to replace the corresponding ThreadLocals with object pools like the ones listed on https://mvnrepository.com/open-source/object-pools.  The choice of a particular framework could be based on benchmarks like https://medium.com/@chrishantha/benchmarking-object-pools-6df007a31ada (my current favorite is Vibur).

Any suggestions / objections / thoughts?  A timely response would be highly appreciated.

Thank you and best regards

Dmytro

[Ralf Steppacher]

I am a little late to the party, but here is my two cents anyway... 

On Thursday, May 17, 2018 at 10:48:28 AM UTC+2, Dmytro Rud wrote:

ThreadLocals are used at different places in IPF -- in the most cases, the only purpose of it is to hold non thread-safe objects like DOM builders.  But the threads, from where the ThreadLocals are used, are per se not pooled, therefore the objects stored in TheradLocals are effectively not reused -- every call to get() results in a call to initialValue().

[..] 

Any suggestions / objections / thoughts?  A timely response would be highly appreciated.

In my opinion the assumption that the threads are not being pooled is wrong, assuming that IPF is used in a Camel context. Camel itself uses thread pools (executor service) by default. And many of the endpoint/component implementations use thread pools (everything Jetty based, everything Netty based, AMQ, etc.). Moving from thread-locals to pooled objects makes it a lot harder to reason about the maximum degree of parallelism because both the size of the object and thread pools has to be known and should be in sync.

I have the feeling that we are trying to pro-actively solve a performance problem for downstream projects that might not even be one. But we add the complexity of managing pool sizes.

Looking at where the pool is used in the 3.5 branch:

WS Client Instances

As far as I know pretty much all cost incurred here is by retrieving the service WSDL and the Schema and the verification of whether the Java stubs comply to the contract before returning an instance of client. However, CXF does cache the expensive objects internally (with varying degrees of success, depending on the exact version of CXF). E.g. it retrieves and parses a (remote) WSDL and schema for a given endpoint only once, then re-uses the cached versions. What's left is the actual HTTP connection for each individual client instance. That again is wrapped in the logic to create the respective WS Camel endpoint. And Camel by default caches endpoints. 

Personally I think that in most usage/load scenarios employing the pool does not improve/change performance in a significant way. But it makes reasoning about the different levels of caching more complicated. 

Document Builder Instances

Honestly, I don't know how expensive it is to create document builder instances. But DOM parsing and management, including the GC overhead created by frequent DOM parsing, is several magnitudes more expensive compared to using SAX or StAX parsing. Both in terms of CPU and memory usage. If a downstream project had any performance issues in or around the IPF document parsing, then the pooled document builder instances probably won't have any significant impact on it. The problem comes with the concept and to fix it would mean to switch to StAX/SAX parsing.

In short, I would vote for removing the object pool again.

Greets,

Ralf

[Dmytro Rud]

Hello Ralf

Thank you for the thorough analysis.  DOM builders are of course not so important, but regarding the WS Client instances I do not agree.  The method org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory#getClient is called from org.openehealth.ipf.platform.camel.ihe.ws.AbstractWsProducer#process, therefore producer pooling is not relevant.  And the way the WS Clients are created (org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory.PortFactory#create) does not involve any caching.  All this code is pretty old (2009), therefore I am not going to change it without an urgent need.  But you may submit a pull request, of course :-)

Best regards

Dmytro

--
You received this message because you are subscribed to the Google Groups "ipf-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ipf-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ralf Steppacher]

Hi Dmytro,

The method org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory#getClient is called from org.openehealth.ipf.platform.camel.ihe.ws.AbstractWsProducer#process, therefore producer pooling is not relevant. 

Yes, agreed, the endpoint with the client connection has to be threadsafe and endpoint caching has nothing to do with that. Still, I'd argue that the previous solution with the thread-local client was the better solution. Because the threads are pooled. Now you need to keep the size of two pools in sync and you incur the (most likely irrelevant) performance overhead of synchronization of the pool.

 

And the way the WS Clients are created (org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory.PortFactory#create) does not involve any caching. 

Ultimately the code will delegate to CXF to create ports, endpoints, and client instances. And CXF already applies caching for expensive but thread-safe objects. The client connection has to be managed in a thread-safe way, of course.

 

All this code is pretty old (2009), therefore I am not going to change it without an urgent need.  But you may submit a pull request, of course :-)

 Well, you changed it just a couple of months ago to switch from thread-locals to an object pool. ;-)  All I am arguing for is that in my opinion, the previous solution was more straight forward because it involved fewer moving parts that have to be kept in sync and most likely performed just as well.

Greets,

Ralf

[Dmytro Rud]

Hi Ralf

Am Di., 6. Nov. 2018 um 08:48 Uhr schrieb Ralf Steppacher <ralf.st...@swisscom.com>:

The method org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory#getClient is called from org.openehealth.ipf.platform.camel.ihe.ws.AbstractWsProducer#process, therefore producer pooling is not relevant. 

Yes, agreed, the endpoint with the client connection has to be threadsafe and endpoint caching has nothing to do with that. Still, I'd argue that the previous solution with the thread-local client was the better solution. Because the threads are pooled. Now you need to keep the size of two pools in sync and you incur the (most likely irrelevant) performance overhead of synchronization of the pool.

Imagine that an application intensively communicates with 5 endpoints, the thread pool size equals to 10, and different threads do not try to access the same endpoint simultaneously.  With ThreadLocal, sooner or later we will have 5*10=50 client objects which will be never evicted.  With ObjectPool, there will be only 5 of them.

In other words, ThreadLocal causes a redundant "saturation" of the threads, while the ObejctPool does not.

 

And the way the WS Clients are created (org.openehealth.ipf.commons.ihe.ws.JaxWsClientFactory.PortFactory#create) does not involve any caching. 

Ultimately the code will delegate to CXF to create ports, endpoints, and client instances. And CXF already applies caching for expensive but thread-safe objects. The client connection has to be managed in a thread-safe way, of course.

 

All this code is pretty old (2009), therefore I am not going to change it without an urgent need.  But you may submit a pull request, of course :-)

 Well, you changed it just a couple of months ago to switch from thread-locals to an object pool. ;-) 

It was urgent ;-)

 

All I am arguing for is that in my opinion, the previous solution was more straight forward because it involved fewer moving parts that have to be kept in sync and most likely performed just as well.

 

Best regards

Dmytro 

[Ralf Steppacher]

Imagine that an application intensively communicates with 5 endpoints, the thread pool size equals to 10, and different threads do not try to access the same endpoint simultaneously.

If it is guaranteed that the threads do not access the same endpoint at the same time then some sort of synchronization is going on that effectively serializes execution, reducing the effective max degree of parallelism to 1. Does it not? That would be a very undesirable scenario.

Anyway, the 50 objects are no problem. Their nature might be though, because they encapsulate an HTTP or HTTPs connection. I'd have to build a small prototype to see what's actually going on on the wire with Camel and CXF in such a scenario... 

 

  With ThreadLocal, sooner or later we will have 5*10=50 client objects which will be never evicted.  With ObjectPool, there will be only 5 of them.

In a scenario where all threads compete for all resources in a more or less evenly distributed manner you will end up with a pool size close to 50 as well. Of course you can limit the size of the pool, but then you are effectively reducing the max degree of parallelism and you should rather reduce the number of threads.

I think the problem here is that we are making assumptions about the workload patterns and problems of a downstream project, which we really should not, and then force a solution in IPF instead of offering the flexibility to the downstream project to apply whatever solution they see fit. Both with the Threadlocal and the pool solution. 

Let's wait for someone to proof with a working example that the current implementation actually causes a problem...

Greets,

Ralf