Rewrite my menu preferences to avoid ProjectState caching

Storing individual user menus inside of the All-Users project state
object leads to unconstrained server memory growth. As users load
preferences the ProjctState object retains all of their preferences.
The cache is only cleared when refs/meta/config within All-Users is
modified, which is very infrequent.

Instead of abusing the ProjectLevelConfig for user preferences,
build a new type VersionedAccountPreferences from the base helper
VersionedMetaData. Load and store all preferences dynamically from
Git. This is similar to the model change metadata will use in the
future, so flushing out any performance problems now with simpler
models like the user preference store is valuable.

User preferences are not accessed very frequently; only at initial
tab load and if the user modifies their settings. If there are any
current performance problems with this storage model the current
impact will be slight while the issue is worked on.

Change-Id: I74c1aed4119b48557e954cc602eb1cf178d201bb

gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java

@@ -28,22 +28,27 @@ import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DiffView;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.TimeFormat;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 
 public class GetPreferences implements RestReadView<AccountResource> {
-  public static final String PREFERENCES = "preferences.config";
+  private static final Logger log = LoggerFactory.getLogger(GetPreferences.class);
+
   public static final String MY = "my";
   public static final String KEY_URL = "url";
   public static final String KEY_TARGET = "target";
@@ -51,19 +56,26 @@ public class GetPreferences implements RestReadView<AccountResource> {
 
   private final Provider<CurrentUser> self;
   private final Provider<ReviewDb> db;
-  private final ProjectState allUsers;
+  private final AllUsersName allUsersName;
+  private final GitRepositoryManager gitMgr;
 
   @Inject
   GetPreferences(Provider<CurrentUser> self, Provider<ReviewDb> db,
-      ProjectCache projectCache) {
+      AllUsersName allUsersName,
+      GitRepositoryManager gitMgr) {
     this.self = self;
     this.db = db;
-    this.allUsers = projectCache.getAllUsers();
+    this.allUsersName = allUsersName;
+    this.gitMgr = gitMgr;
   }
 
   @Override
   public PreferenceInfo apply(AccountResource rsrc)
-      throws AuthException, ResourceNotFoundException, OrmException {
+      throws AuthException,
+      ResourceNotFoundException,
+      OrmException,
+      IOException,
+      ConfigInvalidException {
     if (self.get() != rsrc.getUser()
         && !self.get().getCapabilities().canAdministrateServer()) {
       throw new AuthException("restricted to administrator");
@@ -72,8 +84,16 @@ public class GetPreferences implements RestReadView<AccountResource> {
     if (a == null) {
       throw new ResourceNotFoundException();
     }
-    return new PreferenceInfo(a.getGeneralPreferences(),
-        rsrc.getUser().getAccountId(), allUsers);
+
+    Repository git = gitMgr.openRepository(allUsersName);
+    try {
+      VersionedAccountPreferences p =
+          VersionedAccountPreferences.forUser(rsrc.getUser().getAccountId());
+      p.load(git);
+      return new PreferenceInfo(a.getGeneralPreferences(), p, git);
+    } finally {
+      git.close();
+    }
   }
 
   public static class PreferenceInfo {
@@ -96,13 +116,8 @@ public class GetPreferences implements RestReadView<AccountResource> {
     ChangeScreen changeScreen;
     List<TopMenu.MenuItem> my;
 
-    public PreferenceInfo(AccountGeneralPreferences p, Account.Id accountId,
-        ProjectState allUsers) {
-      this(p, RefNames.refsUsers(accountId), allUsers);
-    }
-
-    public PreferenceInfo(AccountGeneralPreferences p, String ref,
-        ProjectState allUsers) {
+    public PreferenceInfo(AccountGeneralPreferences p,
+        VersionedAccountPreferences v, Repository allUsers) {
       if (p != null) {
         changesPerPage = p.getMaximumPageSize();
         showSiteHeader = p.isShowSiteHeader() ? true : null;
@@ -120,13 +135,20 @@ public class GetPreferences implements RestReadView<AccountResource> {
         diffView = p.getDiffView();
         changeScreen = p.getChangeScreen();
       }
-      my = my(ref, allUsers);
+      my = my(v, allUsers);
     }
 
-    private List<TopMenu.MenuItem> my(String ref, ProjectState allUsers) {
-      List<TopMenu.MenuItem> my = my(allUsers, ref);
-      if (my.isEmpty() && !ref.equals(RefNames.REFS_USER + "default")) {
-        my = my(allUsers, RefNames.REFS_USER + "default");
+    private List<TopMenu.MenuItem> my(VersionedAccountPreferences v,
+        Repository allUsers) {
+      List<TopMenu.MenuItem> my = my(v);
+      if (my.isEmpty() && !v.isDefaults()) {
+        try {
+          VersionedAccountPreferences d = VersionedAccountPreferences.forDefault();
+          d.load(allUsers);
+          my = my(d);
+        } catch (ConfigInvalidException | IOException e) {
+          log.warn("cannot read default preferences", e);
+        }
       }
       if (my.isEmpty()) {
         my.add(new TopMenu.MenuItem("Changes", "#/dashboard/self", null));
@@ -138,9 +160,9 @@ public class GetPreferences implements RestReadView<AccountResource> {
       return my;
     }
 
-    private List<TopMenu.MenuItem> my(ProjectState allUsers, String ref) {
+    private List<TopMenu.MenuItem> my(VersionedAccountPreferences v) {
       List<TopMenu.MenuItem> my = new ArrayList<>();
-      Config cfg = allUsers.getConfig(PREFERENCES, ref).get();
+      Config cfg = v.getConfig();
       for (String subsection : cfg.getSubsections(MY)) {
         String url = my(cfg, subsection, KEY_URL, "#/");
         String target = my(cfg, subsection, KEY_TARGET,

gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java

@@ -18,7 +18,6 @@ import static com.google.gerrit.server.account.GetPreferences.KEY_ID;
 import static com.google.gerrit.server.account.GetPreferences.KEY_TARGET;
 import static com.google.gerrit.server.account.GetPreferences.KEY_URL;
 import static com.google.gerrit.server.account.GetPreferences.MY;
-import static com.google.gerrit.server.account.GetPreferences.PREFERENCES;
 
 import com.google.common.base.Strings;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -34,18 +33,16 @@ import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DiffView;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.TimeFormat;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.SetPreferences.Input;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.MetaDataUpdate;
-import com.google.gerrit.server.git.ProjectLevelConfig;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 
 import java.io.IOException;
@@ -76,22 +73,22 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
   private final AccountCache cache;
   private final ReviewDb db;
   private final MetaDataUpdate.User metaDataUpdateFactory;
-  private final ProjectState allUsers;
+  private final AllUsersName allUsersName;
 
   @Inject
   SetPreferences(Provider<CurrentUser> self, AccountCache cache, ReviewDb db,
-      MetaDataUpdate.User metaDataUpdateFactory, ProjectCache projectCache) {
+      MetaDataUpdate.User metaDataUpdateFactory, AllUsersName allUsersName) {
     this.self = self;
     this.cache = cache;
     this.db = db;
     this.metaDataUpdateFactory = metaDataUpdateFactory;
-    this.allUsers = projectCache.getAllUsers();
+    this.allUsersName = allUsersName;
   }
 
   @Override
   public GetPreferences.PreferenceInfo apply(AccountResource rsrc, Input i)
       throws AuthException, ResourceNotFoundException, OrmException,
-      IOException {
+      IOException, ConfigInvalidException {
     if (self.get() != rsrc.getUser()
         && !self.get().getCapabilities().canAdministrateServer()) {
       throw new AuthException("restricted to administrator");
@@ -102,6 +99,8 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
 
     Account.Id accountId = rsrc.getUser().getAccountId();
     AccountGeneralPreferences p;
+    VersionedAccountPreferences versionedPrefs;
+    MetaDataUpdate md = metaDataUpdateFactory.create(allUsersName);
     db.accounts().beginTransaction(accountId);
     try {
       Account a = db.accounts().get(accountId);
@@ -109,6 +108,9 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
         throw new ResourceNotFoundException();
       }
 
+      versionedPrefs = VersionedAccountPreferences.forUser(accountId);
+      versionedPrefs.load(md);
+
       p = a.getGeneralPreferences();
       if (p == null) {
         p = new AccountGeneralPreferences();
@@ -163,24 +165,21 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
 
       db.accounts().update(Collections.singleton(a));
       db.commit();
-      storeMyMenus(accountId, i.my);
+      storeMyMenus(versionedPrefs, i.my);
+      versionedPrefs.commit(md);
       cache.evict(accountId);
+      return new GetPreferences.PreferenceInfo(
+          p, versionedPrefs,
+          md.getRepository());
     } finally {
+      md.close();
       db.rollback();
     }
-    return new GetPreferences.PreferenceInfo(p, accountId, allUsers);
   }
 
-  private void storeMyMenus(Account.Id accountId, List<TopMenu.MenuItem> my)
-      throws IOException {
-    storeMyMenus(RefNames.refsUsers(accountId), my);
-  }
-
-  public void storeMyMenus(String ref, List<TopMenu.MenuItem> my)
-      throws IOException {
-    ProjectLevelConfig prefsCfg =
-        allUsers.getConfig(PREFERENCES, ref);
-    Config cfg = prefsCfg.get();
+  public static void storeMyMenus(VersionedAccountPreferences prefs,
+      List<TopMenu.MenuItem> my) {
+    Config cfg = prefs.getConfig();
     if (my != null) {
       unsetSection(cfg, MY);
       for (TopMenu.MenuItem item : my) {
@@ -189,10 +188,6 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
         set(cfg, item.name, KEY_ID, item.id);
       }
     }
-    MetaDataUpdate md =
-        metaDataUpdateFactory.create(allUsers.getProject().getNameKey());
-    md.setMessage("Updated preferences\n");
-    prefsCfg.commit(md);
   }
 
   private static void set(Config cfg, String section, String key, String val) {

gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountPreferences.java

@@ -0,0 +1,75 @@
+// Copyright (C) 2014 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.common.base.Strings;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.git.VersionedMetaData;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Config;
+
+import java.io.IOException;
+
+/** Preferences for user accounts. */
+public class VersionedAccountPreferences extends VersionedMetaData {
+  private static final String REFS_USER_DEFAULT = RefNames.REFS_USER + "default";
+  private static final String PREFERENCES = "preferences.config";
+
+  public static VersionedAccountPreferences forUser(Account.Id id) {
+    return new VersionedAccountPreferences(RefNames.refsUsers(id));
+  }
+
+  public static VersionedAccountPreferences forDefault() {
+    return new VersionedAccountPreferences(REFS_USER_DEFAULT);
+  }
+
+  private final String ref;
+  private Config cfg;
+
+  private VersionedAccountPreferences(String ref) {
+    this.ref = ref;
+  }
+
+  public boolean isDefaults() {
+    return REFS_USER_DEFAULT.equals(getRefName());
+  }
+
+  @Override
+  protected String getRefName() {
+    return ref;
+  }
+
+  public Config getConfig() {
+    return cfg;
+  }
+
+  @Override
+  protected void onLoad() throws IOException, ConfigInvalidException {
+    cfg = readConfig(PREFERENCES);
+  }
+
+  @Override
+  protected boolean onSave(CommitBuilder commit) throws IOException,
+      ConfigInvalidException {
+    if (Strings.isNullOrEmpty(commit.getMessage())) {
+      commit.setMessage("Updated preferences\n");
+    }
+    saveConfig(PREFERENCES, cfg);
+    return true;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/config/GetPreferences.java

@@ -15,22 +15,38 @@
 package com.google.gerrit.server.config;
 
 import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.account.GetPreferences.PreferenceInfo;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.account.VersionedAccountPreferences;
+import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.inject.Inject;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
+
+import java.io.IOException;
+
 public class GetPreferences implements RestReadView<ConfigResource> {
-  private final ProjectState allUsers;
+  private final AllUsersName allUsersName;
+  private final GitRepositoryManager gitMgr;
 
   @Inject
-  public GetPreferences(ProjectCache projectCache) {
-    this.allUsers = projectCache.getAllUsers();
+  public GetPreferences(AllUsersName allUsersName,
+      GitRepositoryManager gitMgr) {
+    this.allUsersName = allUsersName;
+    this.gitMgr = gitMgr;
   }
 
   @Override
-  public PreferenceInfo apply(ConfigResource rsrc) {
-    return new PreferenceInfo(null, RefNames.REFS_USER + "default", allUsers);
+  public PreferenceInfo apply(ConfigResource rsrc)
+      throws IOException, ConfigInvalidException {
+    Repository git = gitMgr.openRepository(allUsersName);
+    try {
+      VersionedAccountPreferences p =
+          VersionedAccountPreferences.forDefault();
+      p.load(git);
+      return new PreferenceInfo(null, p, git);
+    } finally {
+      git.close();
+    }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/config/SetPreferences.java

@@ -18,29 +18,31 @@ import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.extensions.annotations.RequiresCapability;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
-import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.GetPreferences.PreferenceInfo;
 import com.google.gerrit.server.account.SetPreferences.Input;
+import com.google.gerrit.server.account.VersionedAccountPreferences;
+import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
 
 import java.io.IOException;
 
 @RequiresCapability(GlobalCapability.ADMINISTRATE_SERVER)
 public class SetPreferences implements RestModifyView<ConfigResource, Input> {
-  private final Provider<com.google.gerrit.server.account.SetPreferences> setPreferences;
-  private final Provider<GetPreferences> getDefaultPreferences;
+  private final MetaDataUpdate.User metaDataUpdateFactory;
+  private final AllUsersName allUsersName;
 
   @Inject
-  SetPreferences(
-      Provider<com.google.gerrit.server.account.SetPreferences> setPreferences,
-      Provider<GetPreferences> getDefaultPreferences) {
-    this.setPreferences = setPreferences;
-    this.getDefaultPreferences = getDefaultPreferences;
+  SetPreferences(MetaDataUpdate.User metaDataUpdateFactory,
+      AllUsersName allUsersName) {
+    this.metaDataUpdateFactory = metaDataUpdateFactory;
+    this.allUsersName = allUsersName;
   }
 
   @Override
   public Object apply(ConfigResource rsrc, Input i) throws BadRequestException,
-      IOException {
+      IOException, ConfigInvalidException {
     if (i.changesPerPage != null || i.showSiteHeader != null
         || i.useFlashClipboard != null || i.downloadScheme != null
         || i.downloadCommand != null || i.copySelfOnEmail != null
@@ -53,9 +55,17 @@ public class SetPreferences implements RestModifyView<ConfigResource, Input> {
         || i.changeScreen != null) {
       throw new BadRequestException("unsupported option");
     }
-    if (i.my != null) {
-      setPreferences.get().storeMyMenus(RefNames.REFS_USER + "default", i.my);
+
+    VersionedAccountPreferences p;
+    MetaDataUpdate md = metaDataUpdateFactory.create(allUsersName);
+    try {
+      p = VersionedAccountPreferences.forDefault();
+      p.load(md);
+      com.google.gerrit.server.account.SetPreferences.storeMyMenus(p, i.my);
+      p.commit(md);
+      return new PreferenceInfo(null, p, md.getRepository());
+    } finally {
+      md.close();
     }
-    return getDefaultPreferences.get().apply(rsrc);
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/MetaDataUpdate.java

@@ -137,7 +137,7 @@ public class MetaDataUpdate {
     return projectName;
   }
 
-  Repository getRepository() {
+  public Repository getRepository() {
     return db;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectLevelConfig.java

@@ -29,23 +29,17 @@ import java.util.Set;
 /** Configuration file in the projects refs/meta/config branch. */
 public class ProjectLevelConfig extends VersionedMetaData {
   private final String fileName;
-  private final String ref;
   private final ProjectState project;
   private Config cfg;
 
   public ProjectLevelConfig(String fileName, ProjectState project) {
-    this(fileName, RefNames.REFS_CONFIG, project);
-  }
-
-  public ProjectLevelConfig(String fileName, String ref, ProjectState project) {
     this.fileName = fileName;
     this.project = project;
-    this.ref = ref;
   }
 
   @Override
   protected String getRefName() {
-    return ref;
+    return RefNames.REFS_CONFIG;
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java

@@ -87,7 +87,7 @@ public class ProjectState {
   private final List<CommentLinkInfo> commentLinks;
 
   private final ProjectConfig config;
-  private final Map<ConfigKey, ProjectLevelConfig> configs;
+  private final Map<String, ProjectLevelConfig> configs;
   private final Set<AccountGroup.UUID> localOwners;
 
   /** Prolog rule state. */
@@ -223,16 +223,11 @@ public class ProjectState {
   }
 
   public ProjectLevelConfig getConfig(String fileName) {
-    return getConfig(fileName, RefNames.REFS_CONFIG);
-  }
-
-  public ProjectLevelConfig getConfig(String fileName, String ref) {
-    ConfigKey cfgKey = new ConfigKey(fileName, ref);
-    if (configs.containsKey(cfgKey)) {
-      return configs.get(cfgKey);
+    if (configs.containsKey(fileName)) {
+      return configs.get(fileName);
     }
 
-    ProjectLevelConfig cfg = new ProjectLevelConfig(fileName, ref, this);
+    ProjectLevelConfig cfg = new ProjectLevelConfig(fileName, this);
     try {
       Repository git = gitMgr.openRepository(getProject().getNameKey());
       try {
@@ -246,7 +241,7 @@ public class ProjectState {
       log.warn("Failed to load " + fileName + " for " + getProject().getName(), e);
     }
 
-    configs.put(cfgKey, cfg);
+    configs.put(fileName, cfg);
     return cfg;
   }
 
@@ -505,26 +500,4 @@ public class ProjectState {
     }
     return false;
   }
-
-  private static class ConfigKey {
-    final String file;
-    final String ref;
-
-    ConfigKey(String file, String ref) {
-      this.file = file;
-      this.ref = ref;
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      return other instanceof ConfigKey
-          && file.equals(((ConfigKey) other).file)
-          && ref.equals(((ConfigKey) other).ref);
-    }
-
-    @Override
-    public int hashCode() {
-      return file.hashCode() * 31 + ref.hashCode();
-    }
-  }
 }