[MAGNOLIA-7994] Remove synchronisation block from ModuleInstanceProvider API Created: 11/Feb/21  Updated: 29/Jan/24  Resolved: 22/Nov/21

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: None
Fix Version/s: 6.2.14

Type: Improvement Priority: Neutral
Reporter: Aleksandr Pchelintcev Assignee: Federico Grilli
Resolution: Done Votes: 0
Labels: performance
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[X]* Doc/release notes changes? Comment present?
[X]* Downstream builds green?
[X]* Solution information and context easily available?
[X]* Tests
[X]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Documentation update required:
Yes
Date of First Response:
Epic Link: Throughput improvements

 Description   

 Problem

public class ModuleInstanceProvider<T> implements Provider<T> {

public synchronized T get() {
    T moduleInstance = (T) moduleRegistry.getModuleInstance(moduleName);
    if (moduleInstance == null) {
        throw new ProvisionException("..");
    }
    return moduleInstance;
}

code- the API unblocked

  • module registry to be changed to use `ConcurrentHashMap`

Dev notes

See the attached branch with the draft solution.
Caveat: we used to use linked hash maps there, so with the above change we lose the order, so the more accurate fix is needed!

Update

Since apparently there is no HashMap implementation which is at the same time thread-safe and able to keep the insertion order, I tried to wrap LinkedHashMap in Collections.synchronizedMap but running again load tests did not show any improvement: the bottleneck now being shifted to Collections$SynchronizedMap.get. This is because synchronizedMap apparently synchronises reads too.

Implemented solution

Keep using LinkedHashSet and let ReentrantReadWriteLock handle synchronisation: this allows concurrent reads unless one writing thread is holding the lock.

Ran a stress test against latest dx-core-demo 6.2.14-SNAPSHOT (as configured out of the box) and profiled Tomcat instance with JProfiler.
The bottlenecks at ModuleInstanceProvider and ModuleRegistryImpl are gone and the throughput as measured by Requests/sec. ratio seems to have improved compared to before the changes.

wrk -c200 -t200 -d600s --timeout 20s --latency --header "User-Agent: wrk" http://localhost:8080/magnoliaPublic/travel.htm

Running 10m test @ http://localhost:8080/magnoliaPublic/travel.htm
  200 threads and 200 connections
 ...
  1015507 requests in 10.00m, 24.59GB read
  Socket errors: connect 0, read 0, write 0, timeout 113
Requests/sec:   1692.15
Transfer/sec:     41.95MB

 



 Comments   
Comment by Aleksandr Pchelintcev [ 10/Nov/21 ]

fgrilli wrt absence of the ordered concurrent map implementation, there's probably a couple of ways to mitigate the problem:

  • use an aux collection to maintain the order (creation/registration of the module is a far less frequent operation, so locking such order collection wouldn't be a big deal, right?)
  • just sort the keys in place whenever the `getModuleDefinitons` is invoked ("get all modules" is also a not very frequent operation I reckon)
Comment by Federico Grilli [ 12/Nov/21 ]

apchelintcev Thanks, an auxiliary collection to keep order sounds like a good idea.

Generated at Mon Feb 12 04:28:40 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.