From a4747d57a8db7289b52143ba4de38f6ee1c76e44 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 13 Feb 2018 13:43:40 -0500 Subject: [PATCH] Don't leak AccountConfig references into AccountState In I9b99490 AccountState was changed to lazily load the project watches and preferences by passing a lambda referring to the AccountConfig instance. Unfortunately, AccountConfig holds a reference to an open Repository instance. In an implementation that does not cache these instances, they are potentially expensive to hold, whether in terms of heap or open resources. Plus, they are never closed. Remove the lazy loading and go back to storing the watches and preferences directly. This may be suboptimal memory usage, but in practice will not be as bad as the many MiB of retained heap per entry we're observing on googlesource.com. We still keep the Supplier arguments in the AccountState constructor, to keep this change as simple as possible to fix a production issue; reverting I9b99490 causes many conflicts. If we want to lazily load these fields, we will need to come up with an approach that reopens the repository on demand. Alternatively, if we decide that lazy loading is not worth the effort, we should remove the Suppliers from the AccountState interface. Change-Id: Id020fd89727b0a3377a98cffe8bf2e9cc2731451 --- .../gerrit/server/account/AccountState.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountState.java b/java/com/google/gerrit/server/account/AccountState.java index 34f4eb14ce..c8dc64cb0a 100644 --- a/java/com/google/gerrit/server/account/AccountState.java +++ b/java/com/google/gerrit/server/account/AccountState.java @@ -111,15 +111,24 @@ public class AccountState { ? externalIds.byAccount(account.getId(), extIdsRev.get()) : ImmutableSet.of(); + // Don't leak references to AccountConfig into the AccountState, since it holds a reference to + // an open Repository instance. + // TODO(ekempin): Find a way to lazily compute these that doesn't hold the repo open. + ImmutableMap> projectWatches = + accountConfig.getProjectWatches(); + GeneralPreferencesInfo generalPreferences = accountConfig.getGeneralPreferences(); + DiffPreferencesInfo diffPreferences = accountConfig.getDiffPreferences(); + EditPreferencesInfo editPreferences = accountConfig.getEditPreferences(); + return Optional.of( new AccountState( allUsersName, account, extIds, - Suppliers.memoize(() -> accountConfig.getProjectWatches()), - Suppliers.memoize(() -> accountConfig.getGeneralPreferences()), - Suppliers.memoize(() -> accountConfig.getDiffPreferences()), - Suppliers.memoize(() -> accountConfig.getEditPreferences()))); + Suppliers.ofInstance(projectWatches), + Suppliers.ofInstance(generalPreferences), + Suppliers.ofInstance(diffPreferences), + Suppliers.ofInstance(editPreferences))); } /**