[Draft: DO NOT MERGE] Avoid findRequiredService for performance reason

[klehmann]

Would be ok for me if that has been identified as a performance bottleneck. Less flexible than before, but I don't see us providing/overriding implementations for specific services anytime soon. @jesse-gallagher What about adding a priority to the service implementations (e.g. int value) so that it's possible to override an already registered service implementation by adding a second one with higher priority? I guess the Java services just pick the first match.

[jesse-gallagher]

I was initially wary of packaging these together like this, but I think it'd probably be fine to have this as-is with this change. I think all of these services are extremely likely to be provided independently by each implementation anyway, and any partial implementations could write their own factory that emits some of theirs and some from another implementation.

That said, I wonder also if this could be improved by caching implementations in a static property and only doing the lookup once per service type. I doubt we'll have a situation where we really want to handle a case where available services will change at runtime, and that should be safe as long as all such service classes are stateless.

[napeiAtHcl]

We used to try a fix which adding class cache for findRequiredService and findServices, the performance is better than the original version which calling service.load every-time, but still not as better as the DominoCommonFactory version. FYI..

[jesse-gallagher]

I've tried a variant of this approach in this branch: https://github.com/HCL-TECH-SOFTWARE/domino-jnx/tree/feature/service_caching . Since you have a superior testing apparatus to what I have, could you give it a shot when you have a chance? I hope that it provides comparable performance to the best case that you saw.

This version keeps caches inside JNXServiceFinder of all of the discovered object instances. I found that only DominoClientBuilder was stateful, so I changed that service to be a factory that emits new objects. This way, we only need to instantiate each service type once, and then it'll use those from there on out.

Keeping the static caches coordinated in here will also make it easier for us down the line if we want to change the behavior to, for example, support OSGi use better.

[napeiAtHcl]

I've tried a variant of this approach in this branch: https://github.com/HCL-TECH-SOFTWARE/domino-jnx/tree/feature/service_caching . Since you have a superior testing apparatus to what I have, could you give it a shot when you have a chance? I hope that it provides comparable performance to the best case that you saw.

This version keeps caches inside JNXServiceFinder of all of the discovered object instances. I found that only DominoClientBuilder was stateful, so I changed that service to be a factory that emits new objects. This way, we only need to instantiate each service type once, and then it'll use those from there on out.

Keeping the static caches coordinated in here will also make it easier for us down the line if we want to change the behavior to, for example, support OSGi use better.

sure, will try the fix and let you know the test result

[jesse-gallagher]

Thanks!

[napeiAtHcl]

@jesse-gallagher, we tried your patch this afternoon. It shows significant performance improvement.

Before patch (processed 9795 view query requests in 3 minutes, average response time 364.05 ms)

After patch (processed 15208 view query requests in 3 minutes, average response time 234.34 ms)

I only tried with the test case with view query. Will try more test cases later.

[jesse-gallagher]

That's excellent to see! Thank you.

[napeiAtHcl]

Didn't see significant performance improvement for average response time of other endpoint. But we see lower P95 response time. (95% of the requests have a response time of less than the p95 value)

Before

After

[jesse-gallagher]

This was handled via the alternative approach described above in https://github.com/HCL-TECH-SOFTWARE/domino-jnx/pull/364