AccountConfig: Remove lazy parsing of project watches and preferences

Nobody is making use of this. The only usage was removed by change
Id020fd8972. As explained in the commit message of change Id020fd8972 it
turned out that lazy parsing leads to a memory leak because for this the
reference to AccountConfig needs to be kept in memory and AccountConfig
may need quite a lot of memory since it holds an open Repository
instance.

Lazy parsing was not really needed and turned out to be a classic case
of premature optimization.

Change-Id: I534cfc0eeb01e0ccd4af3df3986fa23112f7cb63
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-02-15 10:11:40 +01:00
parent 33f5bd2506
commit 0dd475241b
3 changed files with 23 additions and 49 deletions

View File

@@ -74,9 +74,6 @@ import org.eclipse.jgit.revwalk.RevSort;
*
* <p>The commit date of the first commit on the user branch is used as registration date of the
* account. The first commit may be an empty commit (since all config files are optional).
*
* <p>By default preferences and project watches are lazily parsed on need. Eager parsing can be
* requested by {@link #setEagerParsing(boolean)}.
*/
public class AccountConfig extends VersionedMetaData implements ValidationError.Sink {
private final Account.Id accountId;
@@ -88,7 +85,6 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
private ProjectWatches projectWatches;
private Preferences preferences;
private Optional<InternalAccountUpdate> accountUpdate = Optional.empty();
private boolean eagerParsing;
private List<ValidationError> validationErrors;
public AccountConfig(Account.Id accountId, Repository allUsersRepo) {
@@ -97,21 +93,6 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
this.ref = RefNames.refsUsers(accountId);
}
/**
* Sets whether all account data should be eagerly parsed.
*
* <p>Eager parsing should only be used if the caller is interested in validation errors for all
* account data (see {@link #getValidationErrors()}.
*
* @param eagerParsing whether all account data should be eagerly parsed
* @return this AccountConfig instance for chaining
*/
public AccountConfig setEagerParsing(boolean eagerParsing) {
checkState(loadedAccountProperties == null, "Account %s already loaded", accountId.get());
this.eagerParsing = eagerParsing;
return this;
}
@Override
protected String getRefName() {
return ref;
@@ -264,10 +245,8 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
Preferences.readDefaultConfig(repo),
this);
if (eagerParsing) {
projectWatches.parse();
preferences.parse();
}
projectWatches.parse();
preferences.parse();
} else {
loadedAccountProperties = Optional.empty();
@@ -365,9 +344,6 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
/**
* Get the validation errors, if any were discovered during parsing the account data.
*
* <p>To get validation errors for all account data request eager parsing before loading the
* account (see {@link #setEagerParsing(boolean)}).
*
* @return list of errors; empty list if there are no errors.
*/
public List<ValidationError> getValidationErrors() {

View File

@@ -19,7 +19,6 @@ 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;
@@ -40,7 +39,6 @@ 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;
@@ -125,10 +123,10 @@ public class AccountState {
allUsersName,
account,
extIds,
Suppliers.ofInstance(projectWatches),
Suppliers.ofInstance(generalPreferences),
Suppliers.ofInstance(diffPreferences),
Suppliers.ofInstance(editPreferences)));
projectWatches,
generalPreferences,
diffPreferences,
editPreferences));
}
/**
@@ -157,30 +155,30 @@ public class AccountState {
allUsersName,
account,
ImmutableSet.copyOf(extIds),
Suppliers.ofInstance(ImmutableMap.of()),
Suppliers.ofInstance(GeneralPreferencesInfo.defaults()),
Suppliers.ofInstance(DiffPreferencesInfo.defaults()),
Suppliers.ofInstance(EditPreferencesInfo.defaults()));
ImmutableMap.of(),
GeneralPreferencesInfo.defaults(),
DiffPreferencesInfo.defaults(),
EditPreferencesInfo.defaults());
}
private final AllUsersName allUsersName;
private final Account account;
private final ImmutableSet<ExternalId> externalIds;
private final Optional<String> userName;
private final Supplier<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches;
private final Supplier<GeneralPreferencesInfo> generalPreferences;
private final Supplier<DiffPreferencesInfo> diffPreferences;
private final Supplier<EditPreferencesInfo> editPreferences;
private final ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches;
private final GeneralPreferencesInfo generalPreferences;
private final DiffPreferencesInfo diffPreferences;
private final EditPreferencesInfo editPreferences;
private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
private AccountState(
AllUsersName allUsersName,
Account account,
ImmutableSet<ExternalId> externalIds,
Supplier<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches,
Supplier<GeneralPreferencesInfo> generalPreferences,
Supplier<DiffPreferencesInfo> diffPreferences,
Supplier<EditPreferencesInfo> editPreferences) {
ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches,
GeneralPreferencesInfo generalPreferences,
DiffPreferencesInfo diffPreferences,
EditPreferencesInfo editPreferences) {
this.allUsersName = allUsersName;
this.account = account;
this.externalIds = externalIds;
@@ -248,22 +246,22 @@ public class AccountState {
/** The project watches of the account. */
public ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> getProjectWatches() {
return projectWatches.get();
return projectWatches;
}
/** The general preferences of the account. */
public GeneralPreferencesInfo getGeneralPreferences() {
return generalPreferences.get();
return generalPreferences;
}
/** The diff preferences of the account. */
public DiffPreferencesInfo getDiffPreferences() {
return diffPreferences.get();
return diffPreferences;
}
/** The edit preferences of the account. */
public EditPreferencesInfo getEditPreferences() {
return editPreferences.get();
return editPreferences;
}
/**

View File

@@ -101,7 +101,7 @@ public class AccountValidator {
throws IOException, ConfigInvalidException {
rw.reset();
AccountConfig accountConfig = new AccountConfig(accountId, repo);
accountConfig.setEagerParsing(true).load(rw, commit);
accountConfig.load(rw, commit);
if (messages != null) {
messages.addAll(
accountConfig