From 9376b40bb089f489a987c67bb65fff8d2bc3da18 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Sun, 16 May 2021 14:01:54 +0700 Subject: [PATCH 1/7] [PERFORMANCE] FlagsFactory::createFlags needlessly call the builder This method is responsible of ~2% of total memory allocation as per async-profiler and the builder is the main guilty: Its advanced flags filtering capability, not needed for a copy use case come at a high cost. --- .../james/mailbox/store/mail/model/FlagsFactory.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/FlagsFactory.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/FlagsFactory.java index c1d9b1556ca..11adcce02a8 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/FlagsFactory.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/FlagsFactory.java @@ -30,8 +30,7 @@ import com.google.common.collect.ImmutableList; public class FlagsFactory { - - private static Flags asFlags(MailboxMessage mailboxMessage, String[] userFlags) { + public static Flags createFlags(MailboxMessage mailboxMessage, String[] userFlags) { final Flags flags = new Flags(); if (mailboxMessage.isAnswered()) { flags.add(Flags.Flag.ANSWERED); @@ -59,13 +58,6 @@ private static Flags asFlags(MailboxMessage mailboxMessage, String[] userFlags) return flags; } - public static Flags createFlags(MailboxMessage mailboxMessage, String[] userFlags) { - return builder() - .flags(asFlags(mailboxMessage, userFlags)) - .addUserFlags(userFlags) - .build(); - } - public static Builder builder() { return new Builder(); } From b9010d29c650bf65e2cbd306f40f6deff97744b0 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Sun, 16 May 2021 14:05:07 +0700 Subject: [PATCH 2/7] [PERFORMANCE] MessageResultImpl should use underlying MailboxMessage By calling `messageMetadata` too frequently we generate too much flag copies that are responsible of over 2% of total memory allocation as per async-profiler. --- .../james/mailbox/store/MessageResultImpl.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageResultImpl.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageResultImpl.java index b52a655cb4b..c7671c9298c 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageResultImpl.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageResultImpl.java @@ -80,32 +80,32 @@ public MailboxId getMailboxId() { @Override public MessageUid getUid() { - return messageMetaData().getUid(); + return message.getUid(); } @Override public MessageId getMessageId() { - return messageMetaData().getMessageId(); + return message.getMessageId(); } @Override public Date getInternalDate() { - return messageMetaData().getInternalDate(); + return message.getInternalDate(); } @Override public Flags getFlags() { - return messageMetaData().getFlags(); + return message.createFlags(); } @Override public ModSeq getModSeq() { - return messageMetaData().getModSeq(); + return message.getModSeq(); } @Override public long getSize() { - return messageMetaData().getSize(); + return message.getFullContentOctets(); } @Override From 9806c81b0afe3bd7acd7d83981fe7a402af54626 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Sun, 16 May 2021 14:09:07 +0700 Subject: [PATCH 3/7] [PERFORMANCE] MessageViewFactory::toHeaderMap was unfolding headers twice Field::getBody already perform the operation hence there is no need for it. Async-profiler indicates we spend 0.2% of the CPU needlessly that way... Minor but always good to take! --- .../jmap/draft/model/message/view/MessageViewFactory.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/message/view/MessageViewFactory.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/message/view/MessageViewFactory.java index a62fe0212de..d0c3e785433 100644 --- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/message/view/MessageViewFactory.java +++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/message/view/MessageViewFactory.java @@ -39,10 +39,11 @@ import org.apache.james.mailbox.model.MailboxId; import org.apache.james.mailbox.model.MessageId; import org.apache.james.mailbox.model.MessageResult; +import org.apache.james.mime4j.codec.DecodeMonitor; +import org.apache.james.mime4j.codec.DecoderUtil; import org.apache.james.mime4j.dom.Message; import org.apache.james.mime4j.stream.Field; import org.apache.james.mime4j.stream.MimeConfig; -import org.apache.james.mime4j.util.MimeUtil; import org.apache.james.util.ReactorUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -111,7 +112,7 @@ static ImmutableMap toHeaderMap(List fields) { Function>, String> bodyConcatenator = fieldListEntry -> fieldListEntry.getValue() .stream() .map(Field::getBody) - .map(MimeUtil::unscrambleHeaderValue) + .map(body -> DecoderUtil.decodeEncodedWords(body, DecodeMonitor.SILENT)) .collect(Collectors.toList()) .stream() .collect(Collectors.joining(JMAP_MULTIVALUED_FIELD_DELIMITER)); From 7745ca5d7e2ce7386baa81f6e49d20fe7cc144b2 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Sun, 16 May 2021 14:29:46 +0700 Subject: [PATCH 4/7] [PERFORMANCE] JMAPServer should generate JMAP routes once It was generating it for each requests. Each endpoints needs to initialize its own URI parser. Also the version was parsed for each routes and not just once per request. --- .../org/apache/james/jmap/JMAPServer.java | 21 +++++++++++++++---- .../org/apache/james/jmap/VersionParser.java | 4 ++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/JMAPServer.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/JMAPServer.java index 6e9b2903970..4a9b86d895e 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/JMAPServer.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/JMAPServer.java @@ -28,10 +28,14 @@ import javax.annotation.PreDestroy; import javax.inject.Inject; +import org.apache.commons.lang3.tuple.Pair; import org.apache.james.lifecycle.api.Startable; import org.apache.james.util.Port; import org.slf4j.LoggerFactory; +import com.github.steveash.guavate.Guavate; +import com.google.common.collect.Multimap; + import reactor.netty.DisposableServer; import reactor.netty.http.server.HttpServer; import reactor.netty.http.server.HttpServerRequest; @@ -40,16 +44,24 @@ public class JMAPServer implements Startable { private static final int RANDOM_PORT = 0; private final JMAPConfiguration configuration; - private final Set jmapRoutesHandlers; private final VersionParser versionParser; + private final Multimap routes; private Optional server; @Inject public JMAPServer(JMAPConfiguration configuration, Set jmapRoutesHandlers, VersionParser versionParser) { this.configuration = configuration; - this.jmapRoutesHandlers = jmapRoutesHandlers; this.versionParser = versionParser; this.server = Optional.empty(); + + this.routes = versionParser.getSupportedVersions() + .stream() + .flatMap(version -> jmapRoutesHandlers.stream() + .flatMap(handler -> handler.routes(version) + .map(route -> Pair.of(version, route)))) + .collect(Guavate.toImmutableListMultimap( + Pair::getKey, + Pair::getValue)); } public Port getPort() { @@ -76,8 +88,9 @@ private boolean wireTapEnabled() { private JMAPRoute.Action handleVersionRoute(HttpServerRequest request) { try { - return jmapRoutesHandlers.stream() - .flatMap(jmapRoutesHandler -> jmapRoutesHandler.routes(versionParser.parseRequestVersionHeader(request))) + Version version = versionParser.parseRequestVersionHeader(request); + + return routes.get(version).stream() .filter(jmapRoute -> jmapRoute.matches(request)) .map(JMAPRoute::getAction) .findFirst() diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java index 1e9c8ac619e..88603e44799 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java @@ -50,6 +50,10 @@ public VersionParser(Set supportedVersions, JMAPConfiguration jmapConfi this.supportedVersions = supportedVersions; } + public Set getSupportedVersions() { + return supportedVersions; + } + @VisibleForTesting Version parse(String version) { Preconditions.checkNotNull(version); From 2a350a8e6fb696c66f217fb40dcc7eca51ff97d1 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Sun, 16 May 2021 22:52:23 +0700 Subject: [PATCH 5/7] [PERFORMANCE] Use guava Precondition formatter String.format was evaluated on each Version and cost 0.26% of total CPU time budget. --- .../org/apache/james/PeriodicalHealthChecksConfiguration.java | 2 +- .../src/main/java/org/apache/james/jmap/api/model/Preview.java | 2 +- .../jmap/src/main/java/org/apache/james/jmap/VersionParser.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/container/guice/guice-common/src/main/java/org/apache/james/PeriodicalHealthChecksConfiguration.java b/server/container/guice/guice-common/src/main/java/org/apache/james/PeriodicalHealthChecksConfiguration.java index beac23b5ab4..ff124e889cc 100644 --- a/server/container/guice/guice-common/src/main/java/org/apache/james/PeriodicalHealthChecksConfiguration.java +++ b/server/container/guice/guice-common/src/main/java/org/apache/james/PeriodicalHealthChecksConfiguration.java @@ -54,7 +54,7 @@ class ReadyToBuild { PeriodicalHealthChecksConfiguration build() { Preconditions.checkArgument(period.compareTo(MINIMAL_HEALTH_CHECK_PERIOD) >= 0, - "'period' must be equal or greater than " + MINIMAL_HEALTH_CHECK_PERIOD.toMillis() + "ms"); + "'period' must be equal or greater than %d ms", MINIMAL_HEALTH_CHECK_PERIOD.toMillis()); return new PeriodicalHealthChecksConfiguration(period); } diff --git a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/model/Preview.java b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/model/Preview.java index 42b2428081a..fd91f50df95 100644 --- a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/model/Preview.java +++ b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/model/Preview.java @@ -103,7 +103,7 @@ private static String truncateToMaxLength(String body) { Preview(String value) { Preconditions.checkNotNull(value); Preconditions.checkArgument(value.length() <= MAX_LENGTH, - String.format("the preview value '%s' has length longer than %d", value, MAX_LENGTH)); + "the preview value '%s' has length longer than %s", value, MAX_LENGTH); this.value = value; } diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java index 88603e44799..f83237e2381 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/VersionParser.java @@ -45,7 +45,7 @@ public class VersionParser { public VersionParser(Set supportedVersions, JMAPConfiguration jmapConfiguration) { this.jmapConfiguration = jmapConfiguration; Preconditions.checkArgument(supportedVersions.contains(jmapConfiguration.getDefaultVersion()), - jmapConfiguration + " is not a supported JMAP version"); + "%s is not a supported JMAP version", jmapConfiguration); this.supportedVersions = supportedVersions; } From 347a84ebe491274e648228f24fd42744ad064d55 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Mon, 17 May 2021 07:50:07 +0700 Subject: [PATCH 6/7] [PERFORMANCE] Mailboxes metadata: Avoid O(n2) algorithm to compute hasChildren 0.56% running an inefficient algorithm to state which mailboxes have kids, in O(n2). While the percentage is low acting on it with likely improve p99 for `Mailbox/get` with many mailboxes and is thus worth writing. --- .../mailbox/store/StoreMailboxManager.java | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java index 5de99f6a946..89f6e0afac6 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java @@ -27,6 +27,7 @@ import java.util.ArrayList; import java.util.EnumSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Function; @@ -97,6 +98,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -660,19 +662,33 @@ private Function, Flux> metadataTransformation(Ma private Function, Flux> withCounters(MailboxSession session, List mailboxes) { MessageMapper messageMapper = mailboxSessionMapperFactory.getMessageMapper(session); + Map parentMap = parentMap(mailboxes, session); int concurrency = 4; return mailboxFlux -> mailboxFlux .flatMap(mailbox -> retrieveCounters(messageMapper, mailbox, session) .map(Throwing.function( - counters -> toMailboxMetadata(session, mailboxes, mailbox, counters)) + counters -> toMailboxMetadata(session, parentMap, mailbox, counters)) .sneakyThrow()), concurrency); } + private Map parentMap(List mailboxes, MailboxSession session) { + return mailboxes.stream().map(Mailbox::generateAssociatedPath) + .flatMap(path -> { + List hierarchyLevels = path.getHierarchyLevels(session.getPathDelimiter()); + return Lists.reverse(hierarchyLevels).stream().skip(1); + }) + .collect(Guavate.toImmutableMap( + Function.identity(), + any -> true, + (a, b) -> true)); + } + private Function, Flux> withoutCounters(MailboxSession session, List mailboxes) { + Map parentMap = parentMap(mailboxes, session); return mailboxFlux -> mailboxFlux .map(Throwing.function( - mailbox -> toMailboxMetadata(session, mailboxes, mailbox, MailboxCounters + mailbox -> toMailboxMetadata(session, parentMap, mailbox, MailboxCounters .builder() .mailboxId(mailbox.getMailboxId()) .count(0) @@ -738,30 +754,25 @@ private Flux getDelegatedMailboxes(MailboxMapper mailboxMapper, Multi .map(Mailbox::getMailboxId); } - private MailboxMetaData toMailboxMetadata(MailboxSession session, List mailboxes, Mailbox mailbox, MailboxCounters counters) throws UnsupportedRightException { + private MailboxMetaData toMailboxMetadata(MailboxSession session, Map parentMap, Mailbox mailbox, MailboxCounters counters) throws UnsupportedRightException { return new MailboxMetaData( mailbox.generateAssociatedPath(), mailbox.getMailboxId(), getDelimiter(), - computeChildren(session, mailboxes, mailbox), + computeChildren(parentMap, mailbox), Selectability.NONE, storeRightManager.getResolvedMailboxACL(mailbox, session), counters); } - private MailboxMetaData.Children computeChildren(MailboxSession session, List potentialChildren, Mailbox mailbox) { - if (hasChildIn(mailbox, potentialChildren, session)) { + private MailboxMetaData.Children computeChildren(Map parentMap, Mailbox mailbox) { + if (parentMap.getOrDefault(mailbox.generateAssociatedPath(), false)) { return MailboxMetaData.Children.HAS_CHILDREN; } else { return MailboxMetaData.Children.HAS_NO_CHILDREN; } } - private boolean hasChildIn(Mailbox parentMailbox, List mailboxesWithPathLike, MailboxSession mailboxSession) { - return mailboxesWithPathLike.stream() - .anyMatch(mailbox -> mailbox.isChildOf(parentMailbox, mailboxSession)); - } - @Override public Flux search(MultimailboxesSearchQuery expression, MailboxSession session, long limit) throws MailboxException { return getInMailboxIds(expression, session) From b64aeb046af8f3e3c5d6d86c6a62b2be565203d9 Mon Sep 17 00:00:00 2001 From: Benoit Tellier Date: Tue, 18 May 2021 12:44:49 +0700 Subject: [PATCH 7/7] [PERFORMANCE] Limit object creation upon JMAP Draft request writing Filters are dependent of client request, requiring ObjectMapper reconfiguration as object mapper configuration changes are not thread safe. However ObjectMapper javadoc states the following: ``` Method is typically used when multiple, differently configured mappers are needed. Although configuration is shared, cached serializers and deserializers are NOT shared, which means that the new instance may be re-configured before use; meaning that it behaves the same way as if an instance was constructed from scratch. ``` This sounds like our use case! This conforts to advices of this page: https://github.com/FasterXML/jackson-docs/wiki/Presentation:-Jackson-Performance ``` Reuse heavy-weight objects: ObjectMapper (data-binding) and JsonFactory (streaming API) ``` --- .../jmap/draft/methods/JmapResponseWriterImpl.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/JmapResponseWriterImpl.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/JmapResponseWriterImpl.java index a0e1dd816ef..d2a346b930e 100644 --- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/JmapResponseWriterImpl.java +++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/JmapResponseWriterImpl.java @@ -40,11 +40,11 @@ public class JmapResponseWriterImpl implements JmapResponseWriter { public static final String PROPERTIES_FILTER = "propertiesFilter"; - private final ObjectMapperFactory objectMapperFactory; + private final ObjectMapper objectMapper; @Inject public JmapResponseWriterImpl(ObjectMapperFactory objectMapperFactory) { - this.objectMapperFactory = objectMapperFactory; + this.objectMapper = objectMapperFactory.forWriting(); } @Override @@ -60,17 +60,13 @@ public Flux formatMethodResponse(Flux jmapResp } private ObjectMapper newConfiguredObjectMapper(JmapResponse jmapResponse) { - ObjectMapper objectMapper = objectMapperFactory.forWriting(); - FilterProvider filterProvider = jmapResponse .getFilterProvider() .orElseGet(SimpleFilterProvider::new) .setDefaultFilter(SimpleBeanPropertyFilter.serializeAll()) .addFilter(PROPERTIES_FILTER, getPropertiesFilter(jmapResponse.getProperties())); - objectMapper.setFilterProvider(filterProvider); - - return objectMapper; + return objectMapper.copy().setFilterProvider(filterProvider); } private PropertyFilter getPropertiesFilter(Optional> properties) {