opendev/gerrit
Code Issues Proposed changes

Don't leak AccountConfig references into AccountState

Browse Source 
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
This commit is contained in:
Dave Borowitz
2018-02-13 13:43:40 -05:00
parent 0cce7da9e7
commit a4747d57a8
1 changed files with 13 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
java/com/google/gerrit/server/account/AccountState.java
View File

@@ -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<ProjectWatchKey, ImmutableSet<NotifyType>> 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)));
}
/**