From 83c7e0523dd73a030f4435420d9333a6c684c64f Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 2 Mar 2023 11:05:04 +0100 Subject: [PATCH] Fix various performance issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Co-authored-by: René Pfeuffer --- .../groovy/com/cloudogu/scm/RunTask.groovy | 1 + gradle/changelog/performance.yaml | 2 + .../java/sonia/scm/store/AbstractStore.java | 8 ++-- .../scm/repository/xml/MetadataStore.java | 2 +- .../scm/repository/xml/PathDatabase.java | 2 +- .../java/sonia/scm/store/CopyOnWrite.java | 41 +++++++++++++++---- .../store/JAXBConfigurationEntryStore.java | 10 ++--- .../scm/store/JAXBConfigurationStore.java | 4 +- .../java/sonia/scm/store/JAXBDataStore.java | 2 +- .../sonia/scm/store/TypedStoreContext.java | 12 +++++- scm-webapp/src/main/conf/logback.serve.xml | 1 + .../scm/group/DefaultGroupCollector.java | 20 ++++----- .../java/sonia/scm/group/UserGroupCache.java | 6 ++- ...BetweenConfigAndConfigEntryUpdateStep.java | 2 +- .../scm/group/DefaultGroupCollectorTest.java | 41 ++++++++++++++----- 15 files changed, 106 insertions(+), 48 deletions(-) create mode 100644 gradle/changelog/performance.yaml diff --git a/build-plugins/src/main/groovy/com/cloudogu/scm/RunTask.groovy b/build-plugins/src/main/groovy/com/cloudogu/scm/RunTask.groovy index 4626467982..7f74f24d62 100644 --- a/build-plugins/src/main/groovy/com/cloudogu/scm/RunTask.groovy +++ b/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 diff --git a/gradle/changelog/performance.yaml b/gradle/changelog/performance.yaml new file mode 100644 index 0000000000..312e27a856 --- /dev/null +++ b/gradle/changelog/performance.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Performance issues from 2.42.x introduced by the permission overview diff --git a/scm-core/src/main/java/sonia/scm/store/AbstractStore.java b/scm-core/src/main/java/sonia/scm/store/AbstractStore.java index 0fa44b218f..760fefb6e9 100644 --- a/scm-core/src/main/java/sonia/scm/store/AbstractStore.java +++ b/scm-core/src/main/java/sonia/scm/store/AbstractStore.java @@ -35,10 +35,8 @@ import java.util.function.BooleanSupplier; */ public abstract class AbstractStore implements ConfigurationStore { - /** - * 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 implements ConfigurationStore { @Override public T get() { - if (storeObject == null) { + if (!storeAlreadyRead) { storeObject = readObject(); + storeAlreadyRead = true; } return storeObject; @@ -61,6 +60,7 @@ public abstract class AbstractStore implements ConfigurationStore { } writeObject(object); this.storeObject = object; + storeAlreadyRead = true; } diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java index d461f2d3c7..2a6562e092 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java +++ b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java @@ -64,7 +64,7 @@ public class MetadataStore implements UpdateStepRepositoryMetadataAccess { ContextEntry.ContextBuilder.entity(Path.class, path.toString()).build(), "failed read repository metadata", ex ); } - }).withLockedFile(path); + }).withLockedFileForRead(path); } void write(Path path, Repository repository) { diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java index b8a27d5279..a9ce085303 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java +++ b/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 { diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java b/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java index 3539406597..567a518fdc 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java +++ b/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 concurrencyLock = Striped.lock(20); + private static final Striped 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 FileLocker compute(Supplier 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); } } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java index bbb1b5dcaa..016ca04cbf 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java @@ -75,7 +75,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore implements ConfigurationEntryStore { entries.clear(); store(); - }).withLockedFile(file); + }).withLockedFileForWrite(file); } @Override @@ -104,7 +104,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore { entries.put(id, item); store(); - }).withLockedFile(file); + }).withLockedFileForWrite(file); } @Override @@ -114,7 +114,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore { entries.remove(id); store(); - }).withLockedFile(file); + }).withLockedFileForWrite(file); } @Override @@ -178,7 +178,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore extends AbstractStore { } return null; } - ).withLockedFile(configFile); + ).withLockedFileForRead(configFile); } @Override @@ -100,6 +100,6 @@ public class JAXBConfigurationStore extends AbstractStore { } catch (IOException e) { throw new StoreException("Failed to delete store object " + configFile.getPath(), e); } - }).withLockedFile(configFile); + }).withLockedFileForWrite(configFile); } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java index 6a392545ec..bed9c430ba 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java @@ -114,6 +114,6 @@ public class JAXBDataStore extends FileBasedStore implements DataStore return context.unmarshall(file); } return null; - }).withLockedFile(file); + }).withLockedFileForRead(file); } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java b/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java index 224946040d..81183cd5db 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java +++ b/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 { private final JAXBContext jaxbContext; private final TypedStoreParameters parameters; + private static final Map, JAXBContext> contextCache = new HashMap<>(); + private TypedStoreContext(JAXBContext jaxbContext, TypedStoreParameters parameters) { this.jaxbContext = jaxbContext; this.parameters = parameters; } static TypedStoreContext of(TypedStoreParameters parameters) { - try { - JAXBContext jaxbContext = JAXBContext.newInstance(parameters.getType()); + JAXBContext jaxbContext = contextCache.computeIfAbsent(parameters.getType(), type -> createJaxbContext(parameters)); return new TypedStoreContext<>(jaxbContext, parameters); + } + + private static JAXBContext createJaxbContext(TypedStoreParameters parameters) { + try { + return JAXBContext.newInstance(parameters.getType()); } catch (JAXBException e) { throw new StoreException("failed to create context for store", e); } diff --git a/scm-webapp/src/main/conf/logback.serve.xml b/scm-webapp/src/main/conf/logback.serve.xml index 003e55936d..e54c938bd3 100644 --- a/scm-webapp/src/main/conf/logback.serve.xml +++ b/scm-webapp/src/main/conf/logback.serve.xml @@ -45,6 +45,7 @@ + diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java index 98e205a61a..8818e27596 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupCollector.java @@ -60,14 +60,14 @@ public class DefaultGroupCollector implements GroupCollector { private final Cache> cache; private final Set groupResolvers; - private final ConfigurationStoreFactory configurationStoreFactory; + private final ConfigurationStore store; @Inject public DefaultGroupCollector(GroupDAO groupDAO, CacheManager cacheManager, Set 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 groups = builder.build(); LOG.debug("collected following groups for principal {}: {}", principal, groups); - ConfigurationStore 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 fromLastLoginPlusInternal(String principal) { - Set cached = new HashSet<>(getPersistentCache(createStore()).get(principal)); + Set cached = new HashSet<>(getPersistentCache().get(principal)); computeInternalGroups(principal).forEach(cached::add); return cached; } - private static UserGroupCache getPersistentCache(ConfigurationStore store) { + private UserGroupCache getPersistentCache() { return store.getOptional().orElseGet(UserGroupCache::new); } - private ConfigurationStore createStore() { - return configurationStoreFactory.withType(UserGroupCache.class).withName("user-group-cache").build(); - } - @Subscribe(async = false) public void clearCacheOnLogOut(LogoutEvent event) { String principal = event.getPrimaryPrincipal(); diff --git a/scm-webapp/src/main/java/sonia/scm/group/UserGroupCache.java b/scm-webapp/src/main/java/sonia/scm/group/UserGroupCache.java index b55ac503a0..0047811708 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/UserGroupCache.java +++ b/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 groups) { + boolean put(String user, Set groups) { if (cache == null) { cache = new HashMap<>(); } + if (groups.equals(cache.get(user))) { + return false; + } cache.put(user, groups); + return true; } } diff --git a/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java b/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java index afb52219b1..b9ba09eb2c 100644 --- a/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java +++ b/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"); diff --git a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java index 4eae81abd7..356f374440 100644 --- a/scm-webapp/src/test/java/sonia/scm/group/DefaultGroupCollectorTest.java +++ b/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 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 groups = collector.collect(PRINCIPAL_ANONYMOUS); + collector.collect(PRINCIPAL_ANONYMOUS); verify(groupResolver, never()).resolve(PRINCIPAL_ANONYMOUS); } @Test void shouldNotCallResolverForSystemAccount() { groupResolvers.add(groupResolver); - Set groups = collector.collect(PRINCIPAL_SYSTEM); + collector.collect(PRINCIPAL_SYSTEM); verify(groupResolver, never()).resolve(PRINCIPAL_SYSTEM); } @Test void shouldNotResolveInternalGroupsForSystemAccount() { - Set 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 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 {