From e35fef56ad2dadfa41fec0eb801c720f248fb03b Mon Sep 17 00:00:00 2001
From: Federico Grilli <federico.grilli@magnolia-cms.com>
Date: Thu, 21 Jan 2021 09:15:00 +0100
Subject: [PATCH] MAGNOLIA-7981 Avoid calling AbstractRegistry#getProvider(..)
 in a loop

* Processing a raw DefinitionProvider with each request in a highly concurrent environment may lead
to a severe performance degradation due to the frequent access to synchronized methods in RegistryMap.
* AbstractRegistry#getProvider entails calls to three sync methods - even worse this is currently done in a loop for each DefinitionProvider id
---
 .../config/registry/AbstractRegistry.java     |  8 +++--
 .../config/registry/AbstractRegistryTest.java | 31 +++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/magnolia-configuration/src/main/java/info/magnolia/config/registry/AbstractRegistry.java b/magnolia-configuration/src/main/java/info/magnolia/config/registry/AbstractRegistry.java
index bbb50f8356..cf5a3f166a 100644
--- a/magnolia-configuration/src/main/java/info/magnolia/config/registry/AbstractRegistry.java
+++ b/magnolia-configuration/src/main/java/info/magnolia/config/registry/AbstractRegistry.java
@@ -116,7 +116,7 @@ public abstract class AbstractRegistry<T> implements Registry<T> {
         if (provider == null) {
             throw new NoSuchDefinitionException(getReferenceId(id));
         }
-        provider =  provider.isValid() ? getDecoratedDefinitionProvider(provider) : provider;
+        provider = provider.isValid() ? getDecoratedDefinitionProvider(provider) : provider;
         return validate(provider);
     }
 
@@ -150,8 +150,10 @@ public abstract class AbstractRegistry<T> implements Registry<T> {
 
     @Override
     public Collection<DefinitionProvider<T>> getAllProviders() {
-        return getAllMetadata().stream()
-                .map(this::getProvider)
+        return registryMap.values()
+                .stream()
+                .map(this::validate)
+                .map(dp -> dp.isValid() ? getDecoratedDefinitionProvider(dp) : dp)
                 .collect(toList());
     }
 
diff --git a/magnolia-configuration/src/test/java/info/magnolia/config/registry/AbstractRegistryTest.java b/magnolia-configuration/src/test/java/info/magnolia/config/registry/AbstractRegistryTest.java
index d2b6edd824..7c027027a9 100644
--- a/magnolia-configuration/src/test/java/info/magnolia/config/registry/AbstractRegistryTest.java
+++ b/magnolia-configuration/src/test/java/info/magnolia/config/registry/AbstractRegistryTest.java
@@ -54,6 +54,9 @@ import info.magnolia.module.ModuleRegistry;
 import info.magnolia.module.model.ModuleDefinition;
 
 import java.util.Collection;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -311,6 +314,34 @@ public class AbstractRegistryTest {
         assertThat((Collection<?>) provider.getProblems(), hasSize(1));
     }
 
+    @Test
+    public void multipleThreadsInvokingGetAllProvidersCompleteWithinExpectedTime() throws InterruptedException {
+        // GIVEN
+        final DefinitionDecorator<DummyThing> decorator = createDefinitionDecoratorForModule("module-a");
+        registry.addDecorator(decorator);
+        registry.addValidator(provider -> Lists.newArrayList(DefinitionProvider.Problem.major().build()));
+
+        int numberOfThreads = 5000;
+        long expectedMaxTimeInMillis = 1000;
+        ExecutorService service = Executors.newFixedThreadPool(100);
+        CountDownLatch latch = new CountDownLatch(numberOfThreads);
+        long start = System.currentTimeMillis();
+
+        // WHEN
+        for (int i = 0; i < numberOfThreads; i++) {
+            service.submit(() -> {
+                registry.getAllProviders();
+                latch.countDown();
+            });
+        }
+        latch.await();
+        long timeInMillis = System.currentTimeMillis() - start;
+        String message = String.format("Expected to run in less than %s ms but took %s ms instead", expectedMaxTimeInMillis, timeInMillis);
+
+        // THEN
+        assertTrue(message, timeInMillis < expectedMaxTimeInMillis);
+    }
+
     private DefinitionDecorator<DummyThing> createDefinitionDecoratorForModule(String sourceModule) {
         final DefinitionDecorator<DummyThing> decorator = mock(DefinitionDecorator.class);
         final DefinitionDecoratorMetadata decoratorMetadata = mock(DefinitionDecoratorMetadata.class);
-- 
2.19.0