AccountState: Parse project watches and general preferences lazily

When an account is loaded with AccountConfig it loads the watch config
and the preference config, but defers parsing them until project watches
and general preferences are accessed. At the moment they are accessed to
create AccountState but we can defer parsing them further until a
caller of AccountState really needs them. Most callers that load
accounts via the AccountCache don't need project watches and general
preferences and hence they can save the parsing effort when the account
is not cached yet.

It's intentional that external IDs are not lazily loaded. External IDs
are retrieved from the external ID cache and hence getting them should
be cheap. Loading them lazily would be a problem because at the point
when the external IDs are accessed the external ID cache may no longer
have the external IDs for that SHA1. Populating the external ID cache
with external IDs for an old SHA1 may drop the external IDs for current
SHA1 from the cache and as result there would be an increased amount of
evictions and loads from the external ID cache.

Change-Id: I9b9949088c838b2e3f85a1c792ee64e2ab3c3f8d
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/pgm/init/InitAdminUser.java

@@ -17,13 +17,10 @@ package com.google.gerrit.pgm.init;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.client.AuthType;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider;
 import com.google.gerrit.pgm.init.api.ConsoleUI;
 import com.google.gerrit.pgm.init.api.InitFlags;
@@ -152,13 +149,7 @@ public class InitAdminUser implements InitStep {
             authorizedKeys.save("Add SSH key for initial admin user\n");
           }
 
-          AccountState as =
+          AccountState as = AccountState.forAccount(new AllUsersName(allUsers.get()), a, extIds);
-              new AccountState(
-                  new AllUsersName(allUsers.get()),
-                  a,
-                  ImmutableSet.copyOf(extIds),
-                  ImmutableMap.of(),
-                  GeneralPreferencesInfo.defaults());
           for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) {
             accountIndex.replace(as);
           }

java/com/google/gerrit/server/account/AccountCacheImpl.java

@@ -18,11 +18,8 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.account.externalids.ExternalIds;
@@ -131,12 +128,7 @@ public class AccountCacheImpl implements AccountCache {
   private AccountState missing(Account.Id accountId) {
     Account account = new Account(accountId, TimeUtil.nowTs());
     account.setActive(false);
-    return new AccountState(
+    return AccountState.forAccount(allUsersName, account);
-        allUsersName,
-        account,
-        ImmutableSet.of(),
-        ImmutableMap.of(),
-        GeneralPreferencesInfo.defaults());
   }
 
   static class ByIdLoader extends CacheLoader<Account.Id, Optional<AccountState>> {

java/com/google/gerrit/server/account/AccountState.java

@@ -19,6 +19,7 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
 
 import com.google.common.base.Function;
 import com.google.common.base.Strings;
+import com.google.common.base.Suppliers;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableMap;
@@ -31,11 +32,15 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.WatchConfig.NotifyType;
 import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
 import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdNotes;
 import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.config.AllUsersName;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Optional;
+import java.util.function.Supplier;
 import org.apache.commons.codec.DecoderException;
+import org.eclipse.jgit.lib.ObjectId;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -52,37 +57,110 @@ public class AccountState {
   public static final Function<AccountState, Account.Id> ACCOUNT_ID_FUNCTION =
       a -> a.getAccount().getId();
 
+  /**
+   * Creates an AccountState from the given account config.
+   *
+   * @param allUsersName the name of the All-Users repository
+   * @param externalIds class to access external IDs
+   * @param accountConfig the account config, must already be loaded
+   * @return the account state, {@link Optional#empty()} if the account doesn't exist
+   * @throws IOException if accessing the external IDs fails
+   */
   public static Optional<AccountState> fromAccountConfig(
       AllUsersName allUsersName, ExternalIds externalIds, AccountConfig accountConfig)
       throws IOException {
+    return fromAccountConfig(allUsersName, externalIds, accountConfig, null);
+  }
+
+  /**
+   * Creates an AccountState from the given account config.
+   *
+   * <p>If external ID notes are provided the revision of the external IDs branch from which the
+   * external IDs for the account should be loaded is taken from the external ID notes. If external
+   * ID notes are not given the revision of the external IDs branch is taken from the account
+   * config. Updating external IDs is done via {@link ExternalIdNotes} and if external IDs were
+   * updated the revision of the external IDs branch in account config is outdated. Hence after
+   * updating external IDs the external ID notes must be provided.
+   *
+   * @param allUsersName the name of the All-Users repository
+   * @param externalIds class to access external IDs
+   * @param accountConfig the account config, must already be loaded
+   * @param extIdNotes external ID notes, must already be loaded, may be {@code null}
+   * @return the account state, {@link Optional#empty()} if the account doesn't exist
+   * @throws IOException if accessing the external IDs fails
+   */
+  public static Optional<AccountState> fromAccountConfig(
+      AllUsersName allUsersName,
+      ExternalIds externalIds,
+      AccountConfig accountConfig,
+      @Nullable ExternalIdNotes extIdNotes)
+      throws IOException {
     if (!accountConfig.getLoadedAccount().isPresent()) {
       return Optional.empty();
     }
     Account account = accountConfig.getLoadedAccount().get();
+
+    Optional<ObjectId> extIdsRev =
+        extIdNotes != null
+            ? Optional.ofNullable(extIdNotes.getRevision())
+            : accountConfig.getExternalIdsRev();
+    ImmutableSet<ExternalId> extIds =
+        extIdsRev.isPresent()
+            ? externalIds.byAccount(account.getId(), extIdsRev.get())
+            : ImmutableSet.of();
+
     return Optional.of(
         new AccountState(
             allUsersName,
             account,
-            accountConfig.getExternalIdsRev().isPresent()
+            extIds,
-                ? externalIds.byAccount(account.getId(), accountConfig.getExternalIdsRev().get())
+            Suppliers.memoize(() -> accountConfig.getProjectWatches()),
-                : ImmutableSet.of(),
+            Suppliers.memoize(() -> accountConfig.getGeneralPreferences())));
-            accountConfig.getProjectWatches(),
+  }
-            accountConfig.getGeneralPreferences()));
+
+  /**
+   * Creates an AccountState for a given account with no external IDs, no project watches and
+   * default preferences.
+   *
+   * @param allUsersName the name of the All-Users repository
+   * @param account the account
+   * @return the account state
+   */
+  public static AccountState forAccount(AllUsersName allUsersName, Account account) {
+    return forAccount(allUsersName, account, ImmutableSet.of());
+  }
+
+  /**
+   * Creates an AccountState for a given account with no project watches and default preferences.
+   *
+   * @param allUsersName the name of the All-Users repository
+   * @param account the account
+   * @param extIds the external IDs
+   * @return the account state
+   */
+  public static AccountState forAccount(
+      AllUsersName allUsersName, Account account, Collection<ExternalId> extIds) {
+    return new AccountState(
+        allUsersName,
+        account,
+        ImmutableSet.copyOf(extIds),
+        Suppliers.ofInstance(ImmutableMap.of()),
+        Suppliers.ofInstance(GeneralPreferencesInfo.defaults()));
   }
 
   private final AllUsersName allUsersName;
   private final Account account;
   private final ImmutableSet<ExternalId> externalIds;
-  private final ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches;
+  private final Supplier<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches;
-  private final GeneralPreferencesInfo generalPreferences;
+  private final Supplier<GeneralPreferencesInfo> generalPreferences;
   private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
 
-  public AccountState(
+  private AccountState(
       AllUsersName allUsersName,
       Account account,
       ImmutableSet<ExternalId> externalIds,
-      ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches,
+      Supplier<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches,
-      GeneralPreferencesInfo generalPreferences) {
+      Supplier<GeneralPreferencesInfo> generalPreferences) {
     this.allUsersName = allUsersName;
     this.account = account;
     this.externalIds = externalIds;
@@ -145,12 +223,12 @@ public class AccountState {
 
   /** The project watches of the account. */
   public ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> getProjectWatches() {
-    return projectWatches;
+    return projectWatches.get();
   }
 
   /** The general preferences of the account. */
   public GeneralPreferencesInfo getGeneralPreferences() {
-    return generalPreferences;
+    return generalPreferences.get();
   }
 
   /**

java/com/google/gerrit/server/account/AccountsUpdate.java

@@ -19,13 +19,10 @@ import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.Runnables;
 import com.google.gerrit.common.Nullable;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
@@ -385,13 +382,7 @@ public class AccountsUpdate {
           AccountConfig accountConfig = read(r, accountId);
           Account account =
               accountConfig.getNewAccount(new Timestamp(committerIdent.getWhen().getTime()));
-          AccountState accountState =
+          AccountState accountState = AccountState.forAccount(allUsersName, account);
-              new AccountState(
-                  allUsersName,
-                  account,
-                  ImmutableSet.of(),
-                  ImmutableMap.of(),
-                  GeneralPreferencesInfo.defaults());
           InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
           updater.update(accountState, updateBuilder);
 
@@ -634,15 +625,8 @@ public class AccountsUpdate {
     }
 
     public AccountState getAccount() throws IOException {
-      Account account = accountConfig.getLoadedAccount().get();
+      return AccountState.fromAccountConfig(allUsersName, externalIds, accountConfig, extIdNotes)
-      return new AccountState(
+          .get();
-          allUsersName,
-          account,
-          extIdNotes.getRevision() != null
-              ? externalIds.byAccount(account.getId(), extIdNotes.getRevision())
-              : ImmutableSet.of(),
-          accountConfig.getProjectWatches(),
-          accountConfig.getGeneralPreferences());
     }
 
     public ExternalIdNotes getExternalIdNotes() {

java/com/google/gerrit/testing/FakeAccountCache.java

@@ -14,11 +14,8 @@
 
 package com.google.gerrit.testing;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
@@ -80,11 +77,6 @@ public class FakeAccountCache implements AccountCache {
   }
 
   private static AccountState newState(Account account) {
-    return new AccountState(
+    return AccountState.forAccount(new AllUsersName(AllUsersNameProvider.DEFAULT), account);
-        new AllUsersName(AllUsersNameProvider.DEFAULT),
-        account,
-        ImmutableSet.of(),
-        ImmutableMap.of(),
-        GeneralPreferencesInfo.defaults());
   }
 }

javatests/com/google/gerrit/server/index/account/AccountFieldTest.java

@@ -18,12 +18,10 @@ import static com.google.common.truth.Truth.assertThat;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.stream.Collectors.toList;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.account.AccountState;
@@ -43,14 +41,7 @@ public class AccountFieldTest extends GerritBaseTests {
     String metaId = "0e39795bb25dc914118224995c53c5c36923a461";
     account.setMetaId(metaId);
     List<String> values =
-        toStrings(
+        toStrings(AccountField.REF_STATE.get(AccountState.forAccount(allUsersName, account)));
-            AccountField.REF_STATE.get(
-                new AccountState(
-                    allUsersName,
-                    account,
-                    ImmutableSet.of(),
-                    ImmutableMap.of(),
-                    GeneralPreferencesInfo.defaults())));
     assertThat(values).hasSize(1);
     String expectedValue =
         allUsersName.get() + ":" + RefNames.refsUsers(account.getId()) + ":" + metaId;
@@ -78,12 +69,7 @@ public class AccountFieldTest extends GerritBaseTests {
     List<String> values =
         toStrings(
             AccountField.EXTERNAL_ID_STATE.get(
-                new AccountState(
+                AccountState.forAccount(null, account, ImmutableSet.of(extId1, extId2))));
-                    null,
-                    account,
-                    ImmutableSet.of(extId1, extId2),
-                    ImmutableMap.of(),
-                    GeneralPreferencesInfo.defaults())));
     String expectedValue1 = extId1.key().sha1().name() + ":" + extId1.blobId().name();
     String expectedValue2 = extId2.key().sha1().name() + ":" + extId2.blobId().name();
     assertThat(values).containsExactly(expectedValue1, expectedValue2);

javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java

@@ -21,10 +21,7 @@ import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.verify;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
@@ -385,11 +382,6 @@ public class FromAddressGeneratorProviderTest {
     final Account account = new Account(userId, TimeUtil.nowTs());
     account.setFullName(name);
     account.setPreferredEmail(email);
-    return new AccountState(
+    return AccountState.forAccount(new AllUsersName(AllUsersNameProvider.DEFAULT), account);
-        new AllUsersName(AllUsersNameProvider.DEFAULT),
-        account,
-        ImmutableSet.of(),
-        ImmutableMap.of(),
-        GeneralPreferencesInfo.defaults());
   }
 }