Fix various performance issues

- Avoiding read attempts for stores that do not exist (AbstractStore).
- Use of ReadWrite locks (everything withLockedFileForRead or withLockedFileForWrite)
- Caching of JAXB Context (TypedStoreContext.java)
- Avoid unnecessary writes to the UserGroupCache

Committed-by: Eduard Heimbuch <eduard.heimbuch@cloudogu.com>
Co-authored-by: René Pfeuffer <rene.pfeuffer@cloudogu.com>

build-plugins/src/main/groovy/com/cloudogu/scm/RunTask.groovy

@@ -120,6 +120,7 @@ class RunTask extends DefaultTask {
     return {
       project.javaexec {
         mainClass.set(ScmServer.name)
+        jvmArgs("-Xverify:none")
         args(new File(project.buildDir, 'server/config.json').toString())
         environment 'NODE_ENV', 'development'
         classpath project.buildscript.configurations.classpath

gradle/changelog/performance.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Performance issues from 2.42.x introduced by the permission overview

scm-core/src/main/java/sonia/scm/store/AbstractStore.java

@@ -35,10 +35,8 @@ import java.util.function.BooleanSupplier;
  */
 public abstract class AbstractStore<T> implements ConfigurationStore<T> {
 
-  /**
-   * stored object
-   */
   protected T storeObject;
+  private boolean storeAlreadyRead = false;
   private final BooleanSupplier readOnly;
 
   protected AbstractStore(BooleanSupplier readOnly) {
@@ -47,8 +45,9 @@ public abstract class AbstractStore<T> implements ConfigurationStore<T> {
 
   @Override
   public T get() {
-    if (storeObject == null) {
+    if (!storeAlreadyRead) {
       storeObject = readObject();
+      storeAlreadyRead = true;
     }
 
     return storeObject;
@@ -61,6 +60,7 @@ public abstract class AbstractStore<T> implements ConfigurationStore<T> {
     }
     writeObject(object);
     this.storeObject = object;
+    storeAlreadyRead = true;
   }
 
 

scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java

@@ -64,7 +64,7 @@ public class MetadataStore implements UpdateStepRepositoryMetadataAccess<Path> {
           ContextEntry.ContextBuilder.entity(Path.class, path.toString()).build(), "failed read repository metadata", ex
         );
       }
-    }).withLockedFile(path);
+    }).withLockedFileForRead(path);
   }
 
   void write(Path path, Repository repository) {

scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java

@@ -146,7 +146,7 @@ class PathDatabase {
           ex
         );
       }
-    }).withLockedFile(storePath);
+    }).withLockedFileForRead(storePath);
   }
 
   private void readRepository(XMLStreamReader reader, OnRepository onRepository) throws XMLStreamException {

scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java

@@ -34,6 +34,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.UUID;
 import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
 import java.util.function.Supplier;
 
 /**
@@ -48,7 +49,7 @@ public final class CopyOnWrite {
 
   private static final Logger LOG = LoggerFactory.getLogger(CopyOnWrite.class);
 
-  private static final Striped<Lock> concurrencyLock = Striped.lock(20);
+  private static final Striped<ReadWriteLock> concurrencyLock = Striped.readWriteLock(100);
 
   private CopyOnWrite() {
   }
@@ -59,7 +60,7 @@ public final class CopyOnWrite {
       Path temporaryFile = createTemporaryFile(targetFile);
       executeCallback(writer, targetFile, temporaryFile);
       replaceOriginalFile(targetFile, temporaryFile);
-    }).withLockedFile(targetFile);
+    }).withLockedFileForWrite(targetFile);
   }
 
   public static <R> FileLocker<R> compute(Supplier<R> supplier) {
@@ -80,21 +81,45 @@ public final class CopyOnWrite {
       this.supplier = supplier;
     }
 
-    public R withLockedFile(Path file) {
-      return withLockedFile(file.toAbsolutePath().toString());
+    public R withLockedFileForRead(Path file) {
+      return withLockedFileForRead(file.toAbsolutePath().toString());
     }
 
-    public R withLockedFile(File file) {
-      return withLockedFile(file.getPath());
+    public R withLockedFileForRead(File file) {
+      return withLockedFileForRead(file.getPath());
     }
 
-    public R withLockedFile(String file) {
-      Lock lock = concurrencyLock.get(file);
+    public R withLockedFileForRead(String file) {
+      LOG.trace("read lock get {}", file);
+      Lock lock = concurrencyLock.get(file).readLock();
+      LOG.trace("read lock passed {}", file);
       lock.lock();
       try {
         return supplier.get();
       } finally {
         lock.unlock();
+        LOG.trace("read lock released {}", file);
+      }
+    }
+
+    public R withLockedFileForWrite(Path file) {
+      return withLockedFileForWrite(file.toAbsolutePath().toString());
+    }
+
+    public R withLockedFileForWrite(File file) {
+      return withLockedFileForWrite(file.getPath());
+    }
+
+    public R withLockedFileForWrite(String file) {
+      LOG.trace("write lock get > {}", file);
+      Lock lock = concurrencyLock.get(file).writeLock();
+      LOG.trace("write lock passed {}", file);
+      lock.lock();
+      try {
+        return supplier.get();
+      } finally {
+        lock.unlock();
+        LOG.trace("write lock released {}", file);
       }
     }
   }

scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java

@@ -75,7 +75,7 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
       if (file.exists()) {
         load();
       }
-    }).withLockedFile(file);
+    }).withLockedFileForRead(file);
   }
 
   @Override
@@ -85,7 +85,7 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
     execute(() -> {
       entries.clear();
       store();
-    }).withLockedFile(file);
+    }).withLockedFileForWrite(file);
   }
 
   @Override
@@ -104,7 +104,7 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
     execute(() -> {
       entries.put(id, item);
       store();
-    }).withLockedFile(file);
+    }).withLockedFileForWrite(file);
   }
 
   @Override
@@ -114,7 +114,7 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
     execute(() -> {
       entries.remove(id);
       store();
-    }).withLockedFile(file);
+    }).withLockedFileForWrite(file);
   }
 
   @Override
@@ -178,7 +178,7 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
             }
           }
         }
-      })).withLockedFile(file);
+      })).withLockedFileForRead(file);
   }
 
   private void store() {

scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationStore.java

@@ -79,7 +79,7 @@ public class JAXBConfigurationStore<T> extends AbstractStore<T> {
         }
         return null;
       }
-    ).withLockedFile(configFile);
+    ).withLockedFileForRead(configFile);
   }
 
   @Override
@@ -100,6 +100,6 @@ public class JAXBConfigurationStore<T> extends AbstractStore<T> {
       } catch (IOException e) {
         throw new StoreException("Failed to delete store object " + configFile.getPath(), e);
       }
-    }).withLockedFile(configFile);
+    }).withLockedFileForWrite(configFile);
   }
 }

scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java

@@ -114,6 +114,6 @@ public class JAXBDataStore<T> extends FileBasedStore<T> implements DataStore<T>
         return context.unmarshall(file);
       }
       return null;
-    }).withLockedFile(file);
+    }).withLockedFileForRead(file);
   }
 }

scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java

@@ -30,6 +30,8 @@ import javax.xml.bind.Marshaller;
 import javax.xml.bind.Unmarshaller;
 import javax.xml.bind.annotation.adapters.XmlAdapter;
 import java.io.File;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -38,15 +40,21 @@ final class TypedStoreContext<T> {
   private final JAXBContext jaxbContext;
   private final TypedStoreParameters<T> parameters;
 
+  private static final Map<Class<?>, JAXBContext> contextCache = new HashMap<>();
+
   private TypedStoreContext(JAXBContext jaxbContext, TypedStoreParameters<T> parameters) {
     this.jaxbContext = jaxbContext;
     this.parameters = parameters;
   }
 
   static <T> TypedStoreContext<T> of(TypedStoreParameters<T> parameters) {
-    try {
-      JAXBContext jaxbContext = JAXBContext.newInstance(parameters.getType());
+      JAXBContext jaxbContext = contextCache.computeIfAbsent(parameters.getType(), type -> createJaxbContext(parameters));
       return new TypedStoreContext<>(jaxbContext, parameters);
+  }
+
+  private static <T> JAXBContext createJaxbContext(TypedStoreParameters<T> parameters) {
+    try {
+      return JAXBContext.newInstance(parameters.getType());
     } catch (JAXBException e) {
       throw new StoreException("failed to create context for store", e);
     }

scm-webapp/src/main/conf/logback.serve.xml

@@ -45,6 +45,7 @@
   <logger name="sonia.scm" level="TRACE" />
   <logger name="com.cloudogu.scm" level="TRACE" />
 
+  <logger name="sonia.scm.store.CopyOnWrite" level="DEBUG" />
   <logger name="sonia.scm.security.AuthorizationCollector" level="DEBUG" />
   <logger name="sonia.scm.web.filter.AutoLoginFilter" level="DEBUG" />
   <logger name="sonia.scm.security.XsrfProtectionFilter" level="DEBUG" />

scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java

@@ -60,14 +60,14 @@ public class DefaultGroupCollector implements GroupCollector {
   private final Cache<String, Set<String>> cache;
   private final Set<GroupResolver> groupResolvers;
 
-  private final ConfigurationStoreFactory configurationStoreFactory;
+  private final ConfigurationStore<UserGroupCache> store;
 
   @Inject
   public DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set<GroupResolver> groupResolvers, ConfigurationStoreFactory configurationStoreFactory) {
     this.groupDAO = groupDAO;
     this.cache = cacheManager.getCache(CACHE_NAME);
     this.groupResolvers = groupResolvers;
-    this.configurationStoreFactory = configurationStoreFactory;
+    this.store = configurationStoreFactory.withType(UserGroupCache.class).withName("user-group-cache").build();
   }
 
   @Override
@@ -87,29 +87,25 @@ public class DefaultGroupCollector implements GroupCollector {
     Set<String> groups = builder.build();
     LOG.debug("collected following groups for principal {}: {}", principal, groups);
 
-    ConfigurationStore<UserGroupCache> store = createStore();
-    UserGroupCache persistentCache = getPersistentCache(store);
-    persistentCache.put(principal, groups);
-    store.set(persistentCache);
+    UserGroupCache persistentCache = getPersistentCache();
+    if (persistentCache.put(principal, groups)) {
+      store.set(persistentCache);
+    }
 
     return groups;
   }
 
   @Override
   public Set<String> fromLastLoginPlusInternal(String principal) {
-    Set<String> cached = new HashSet<>(getPersistentCache(createStore()).get(principal));
+    Set<String> cached = new HashSet<>(getPersistentCache().get(principal));
     computeInternalGroups(principal).forEach(cached::add);
     return cached;
   }
 
-  private static UserGroupCache getPersistentCache(ConfigurationStore<UserGroupCache> store) {
+  private UserGroupCache getPersistentCache() {
     return store.getOptional().orElseGet(UserGroupCache::new);
   }
 
-  private ConfigurationStore<UserGroupCache> createStore() {
-    return configurationStoreFactory.withType(UserGroupCache.class).withName("user-group-cache").build();
-  }
-
   @Subscribe(async = false)
   public void clearCacheOnLogOut(LogoutEvent event) {
     String principal = event.getPrimaryPrincipal();

scm-webapp/src/main/java/sonia/scm/group/UserGroupCache.java

@@ -49,10 +49,14 @@ class UserGroupCache {
     return cache.getOrDefault(user, emptySet());
   }
 
-  void put(String user, Set<String> groups) {
+  boolean put(String user, Set<String> groups) {
     if (cache == null) {
       cache = new HashMap<>();
     }
+    if (groups.equals(cache.get(user))) {
+      return false;
+    }
     cache.put(user, groups);
+    return true;
   }
 }

scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java

@@ -92,7 +92,7 @@ abstract class DifferentiateBetweenConfigAndConfigEntryUpdateStep {
   private void updateSingleFile(Path configFile) {
     LOG.info("Updating config entry file: {}", configFile);
 
-    Document configEntryDocument = compute(() -> readAsXmlDocument(configFile)).withLockedFile(configFile);
+    Document configEntryDocument = compute(() -> readAsXmlDocument(configFile)).withLockedFileForRead(configFile);
 
     configEntryDocument.getDocumentElement().setAttribute("type", "config-entry");
 

scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java

@@ -48,7 +48,10 @@ import java.util.Optional;
 import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static sonia.scm.security.Authentications.PRINCIPAL_ANONYMOUS;
@@ -66,7 +69,7 @@ class DefaultGroupCollectorTest {
   @Mock(answer = Answers.RETURNS_DEEP_STUBS)
   private ConfigurationStoreFactory configurationStoreFactory;
   @Mock(answer = Answers.RETURNS_DEEP_STUBS)
-  private ConfigurationStore configurationStore;
+  private ConfigurationStore<UserGroupCache> configurationStore;
 
   private MapCacheManager mapCacheManager;
 
@@ -74,18 +77,20 @@ class DefaultGroupCollectorTest {
 
   private DefaultGroupCollector collector;
 
+  private UserGroupCache userGroupCache;
+
   @BeforeEach
   void initCollector() {
     groupResolvers = new HashSet<>();
     mapCacheManager = new MapCacheManager();
-    collector = new DefaultGroupCollector(groupDAO, mapCacheManager, groupResolvers, configurationStoreFactory);
-  }
-
-  @BeforeEach
-  void initStore() {
     when(configurationStoreFactory.withType(UserGroupCache.class).withName("user-group-cache").build())
       .thenReturn(configurationStore);
-    when(configurationStore.getOptional()).thenReturn(Optional.empty());
+    when(configurationStore.getOptional()).thenAnswer(invocation -> Optional.ofNullable(userGroupCache));
+    lenient().doAnswer(invocation -> {
+      userGroupCache = invocation.getArgument(0, UserGroupCache.class);
+      return null;
+    }).when(configurationStore).set(any());
+    collector = new DefaultGroupCollector(groupDAO, mapCacheManager, groupResolvers, configurationStoreFactory);
   }
 
   @Test
@@ -140,20 +145,20 @@ class DefaultGroupCollectorTest {
   @Test
   void shouldNotCallResolverForAnonymous() {
     groupResolvers.add(groupResolver);
-    Set<String> groups = collector.collect(PRINCIPAL_ANONYMOUS);
+    collector.collect(PRINCIPAL_ANONYMOUS);
     verify(groupResolver, never()).resolve(PRINCIPAL_ANONYMOUS);
   }
 
   @Test
   void shouldNotCallResolverForSystemAccount() {
     groupResolvers.add(groupResolver);
-    Set<String> groups = collector.collect(PRINCIPAL_SYSTEM);
+    collector.collect(PRINCIPAL_SYSTEM);
     verify(groupResolver, never()).resolve(PRINCIPAL_SYSTEM);
   }
 
   @Test
   void shouldNotResolveInternalGroupsForSystemAccount() {
-    Set<String> groups = collector.collect(PRINCIPAL_SYSTEM);
+    collector.collect(PRINCIPAL_SYSTEM);
     verify(groupDAO, never()).getAll();
   }
 
@@ -168,6 +173,22 @@ class DefaultGroupCollectorTest {
     assertThat(cachedGroups).contains("hog");
   }
 
+  @Test
+  void shouldCacheGroups() {
+    collector.collect("trillian");
+
+    Set<String> groups = userGroupCache.get("trillian");
+    assertThat(groups).contains("_authenticated");
+  }
+
+  @Test
+  void shouldNotPersistUnchangedGroups() {
+    collector.collect("trillian");
+    collector.collect("trillian");
+
+    verify(configurationStore, times(1)).set(any());
+  }
+
   @Nested
   class WithGroupsFromDao {