From 246bb76ab55b6077f47069e4ad41979f81b6d1af Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Tue, 9 Apr 2024 13:18:10 -0600 Subject: Avoid work in Optional.orElse() calls Any code inside the orElse() will always be called, even if the result isn't used because the optional is present. This is wasteful at a minimum and can be actively harmful (as seen in [1]) because of side effects or performance impacts. Fix that by replacing all orElse() calls that create new instances or do non-constant work with either a call to orElseGet() or an if statement. It's possible this will result in performance improvements, but I didn't attempt to measure any, so I'm not including a release note. It would be nice if there were an ErrorProne checker for this, but one doesn't exist yet. [1] https://gerrit-review.googlesource.com/c/gerrit/+/417915/comment/0ea287cd_bfecb7f2/ Change-Id: Icab4115998fb4f3787ca74c0890949cb3ce5c6a6 Release-Notes: skip --- java/com/google/gerrit/acceptance/GerritServer.java | 4 ++-- .../acceptance/testsuite/group/GroupOperationsImpl.java | 2 +- .../acceptance/testsuite/project/ProjectOperationsImpl.java | 2 +- java/com/google/gerrit/httpd/raw/StaticModule.java | 2 +- java/com/google/gerrit/server/DeadlineChecker.java | 12 +++++++----- java/com/google/gerrit/server/account/AccountCacheImpl.java | 2 +- java/com/google/gerrit/server/account/AccountManager.java | 2 +- java/com/google/gerrit/server/change/DeleteReviewerOp.java | 2 +- .../gerrit/server/git/DefaultChangeReportFormatter.java | 2 +- .../com/google/gerrit/server/git/receive/ReceiveCommits.java | 8 ++++---- .../com/google/gerrit/server/group/db/AuditLogFormatter.java | 2 +- java/com/google/gerrit/server/patch/PatchFile.java | 2 +- java/com/google/gerrit/server/project/Reachable.java | 2 +- .../com/google/gerrit/server/group/db/AbstractGroupTest.java | 2 +- 14 files changed, 24 insertions(+), 22 deletions(-) diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index a9ad3f237b..3bbc74ee13 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -332,7 +332,7 @@ public class GerritServer implements AutoCloseable { String configuredIndexBackend = cfg.getString("index", null, "type"); if (configuredIndexBackend == null) { // Propagate index type to pgms that run off of the gerrit.config file on local disk. - IndexType indexType = IndexType.fromEnvironment().orElse(new IndexType("fake")); + IndexType indexType = IndexType.fromEnvironment().orElseGet(() -> new IndexType("fake")); gerritConfig.setString("index", null, "type", indexType.isLucene() ? "lucene" : "fake"); } gerritConfig.save(); @@ -509,7 +509,7 @@ public class GerritServer implements AutoCloseable { IndexType indexType = (configuredIndexBackend != null) ? new IndexType(configuredIndexBackend) - : IndexType.fromEnvironment().orElse(new IndexType("fake")); + : IndexType.fromEnvironment().orElseGet(() -> new IndexType("fake")); daemon.setIndexModule(createIndexModule(indexType, baseConfig, testIndexModule)); daemon.setEnableHttpd(desc.httpd()); diff --git a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java index dcf1158e14..a37c2babbe 100644 --- a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java @@ -78,7 +78,7 @@ public class GroupOperationsImpl implements GroupOperations { private InternalGroupCreation toInternalGroupCreation(TestGroupCreation groupCreation) { AccountGroup.Id groupId = AccountGroup.id(seq.nextGroupId()); - String groupName = groupCreation.name().orElse("group-with-id-" + groupId.get()); + String groupName = groupCreation.name().orElseGet(() -> "group-with-id-" + groupId.get()); AccountGroup.UUID groupUuid = GroupUuid.make(groupName, serverIdent); AccountGroup.NameKey nameKey = AccountGroup.nameKey(groupName); return InternalGroupCreation.builder() diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java index deeb84309f..1014a0f7ad 100644 --- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java @@ -85,7 +85,7 @@ public class ProjectOperationsImpl implements ProjectOperations { } private Project.NameKey createNewProject(TestProjectCreation projectCreation) throws Exception { - String name = projectCreation.name().orElse(RandomStringUtils.randomAlphabetic(8)); + String name = projectCreation.name().orElseGet(() -> RandomStringUtils.randomAlphabetic(8)); CreateProjectArgs args = new CreateProjectArgs(); args.setProjectName(name); diff --git a/java/com/google/gerrit/httpd/raw/StaticModule.java b/java/com/google/gerrit/httpd/raw/StaticModule.java index 15dcf42e0e..1e76f215e9 100644 --- a/java/com/google/gerrit/httpd/raw/StaticModule.java +++ b/java/com/google/gerrit/httpd/raw/StaticModule.java @@ -242,7 +242,7 @@ public class StaticModule extends ServletModule { @GerritServerConfig Config cfg, GerritApi gerritApi, ExperimentFeatures experimentFeatures) { - String cdnPath = options.devCdn().orElse(cfg.getString("gerrit", null, "cdnPath")); + String cdnPath = options.devCdn().orElseGet(() -> cfg.getString("gerrit", null, "cdnPath")); String faviconPath = cfg.getString("gerrit", null, "faviconPath"); return new IndexServlet(canonicalUrl, cdnPath, faviconPath, gerritApi, experimentFeatures); } diff --git a/java/com/google/gerrit/server/DeadlineChecker.java b/java/com/google/gerrit/server/DeadlineChecker.java index f41b1e3c3d..9b7ffe6c3f 100644 --- a/java/com/google/gerrit/server/DeadlineChecker.java +++ b/java/com/google/gerrit/server/DeadlineChecker.java @@ -180,12 +180,14 @@ public class DeadlineChecker implements RequestStateProvider { this.timeoutName = clientedProvidedTimeout .map(clientTimeout -> "client.timeout") - .orElse( - serverSideDeadline - .map(serverDeadline -> serverDeadline.id() + ".timeout") - .orElse("timeout")); + .orElseGet( + () -> + serverSideDeadline + .map(serverDeadline -> serverDeadline.id() + ".timeout") + .orElse("timeout")); this.timeout = - clientedProvidedTimeout.orElse(serverSideDeadline.map(ServerDeadline::timeout).orElse(0L)); + clientedProvidedTimeout.orElseGet( + () -> serverSideDeadline.map(ServerDeadline::timeout).orElse(0L)); this.deadline = timeout > 0 ? Optional.of(start + timeout) : Optional.empty(); } diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index 66a36f6595..d306ad0e92 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -98,7 +98,7 @@ public class AccountCacheImpl implements AccountCache { @Override public AccountState getEvenIfMissing(Account.Id accountId) { - return get(accountId).orElse(missing(accountId)); + return get(accountId).orElseGet(() -> missing(accountId)); } @Override diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index 891a467577..5edba08eaa 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -195,7 +195,7 @@ public class AccountManager { "Unable to deactivate account %s", authRequest .getUserName() - .orElse(" for external ID key " + authRequest.getExternalIdKey().get())); + .orElseGet(() -> " for external ID key " + authRequest.getExternalIdKey().get())); } } diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java index 1199be5657..84ea730c57 100644 --- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java +++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java @@ -214,7 +214,7 @@ public class DeleteReviewerOp extends ReviewerOp { reviewerDeleted.fire( ctx.getChangeData(currChange), patchSet, - accountCache.get(reviewer.id()).orElse(AccountState.forAccount(reviewer)), + accountCache.get(reviewer.id()).orElseGet(() -> AccountState.forAccount(reviewer)), ctx.getAccount(), mailMessage, newApprovals, diff --git a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java index 4f6094e685..58c3eb10b9 100644 --- a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java +++ b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java @@ -54,7 +54,7 @@ public class DefaultChangeReportFormatter implements ChangeReportFormatter { urlFormatter .get() .getChangeViewUrl(c.getProject(), c.getId()) - .orElse(c.getId().toString())); + .orElseGet(() -> c.getId().toString())); } protected String cropSubject(String subject) { diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 0f5e3bc1ba..dbe0fa8857 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -2019,10 +2019,10 @@ class ReceiveCommits { magicBranch.dest = BranchNameKey.create(project.getNameKey(), ref); magicBranch.perm = permissions.ref(ref); - Optional err = - checkRefPermission(magicBranch.perm, RefPermission.READ) - .map(Optional::of) - .orElse(checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE)); + Optional err = checkRefPermission(magicBranch.perm, RefPermission.READ); + if (err.isEmpty()) { + err = checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE); + } if (err.isPresent()) { rejectProhibited(cmd, err.get()); return; diff --git a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java index 235ca4f613..3ba087e315 100644 --- a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java +++ b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java @@ -167,7 +167,7 @@ public class AuditLogFormatter { .map(Account::getName) // Historically, the database did not enforce relational integrity, so it is // possible for groups to have non-existing members. - .orElse("No Account for Id #" + accountId); + .orElseGet(() -> "No Account for Id #" + accountId); } private PersonIdent getParsableAuthorIdent( diff --git a/java/com/google/gerrit/server/patch/PatchFile.java b/java/com/google/gerrit/server/patch/PatchFile.java index 7a8180bd13..c3a6807fac 100644 --- a/java/com/google/gerrit/server/patch/PatchFile.java +++ b/java/com/google/gerrit/server/patch/PatchFile.java @@ -61,7 +61,7 @@ public class PatchFile { .filter(f -> f.getKey().equals(fileName)) .map(Map.Entry::getValue) .findFirst() - .orElse(FileDiffOutput.empty(fileName, ObjectId.zeroId(), ObjectId.zeroId())); + .orElseGet(() -> FileDiffOutput.empty(fileName, ObjectId.zeroId(), ObjectId.zeroId())); if (Patch.PATCHSET_LEVEL.equals(fileName)) { aTree = null; diff --git a/java/com/google/gerrit/server/project/Reachable.java b/java/com/google/gerrit/server/project/Reachable.java index 342c2bcc06..c935adf079 100644 --- a/java/com/google/gerrit/server/project/Reachable.java +++ b/java/com/google/gerrit/server/project/Reachable.java @@ -74,7 +74,7 @@ public class Reachable { Collection filtered = optionalUserProvider .map(permissionBackend::user) - .orElse(permissionBackend.currentUser()) + .orElseGet(() -> permissionBackend.currentUser()) .project(project) .filter(refs, repo, RefFilterOptions.defaults()); Collection visible = new ArrayList<>(); diff --git a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java index bea5eaad50..a9ceddb076 100644 --- a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java +++ b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java @@ -141,7 +141,7 @@ public class AbstractGroupTest { return GroupConfig.loadForGroup(allUsersName, allUsersRepo, uuid) .getLoadedGroup() .map(InternalGroup::getName) - .orElse("Group " + uuid); + .orElseGet(() -> "Group " + uuid); } catch (IOException | ConfigInvalidException e) { return "Group " + uuid; } -- cgit v1.2.3