Skip to content

Commit

Permalink
perf: fix performance regression with cache since we added getConfigR…
Browse files Browse the repository at this point in the history 
…esult, as we were caching the GResultOf<Object>. This was causing extra calls to get the result after getting data from the cache. We now have a seperate cache for the normal flow and one for GResultOf<Object>.
  • Loading branch information
@credmond-git
credmond-git committed Nov 28, 2024
1 parent ea3eb75 commit 706c1cd
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 29 deletions.
129 changes: 101 additions & 28 deletions gestalt-core/src/main/java/org/github/gestalt/config/GestaltCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
@SuppressWarnings("OverloadMethodsDeclarationOrder")
public class GestaltCache implements Gestalt, CoreReloadListener {
private final Gestalt delegate;
private final Map<Triple<String, TypeCapture<?>, Tags>, GResultOf<Object>> cache = Collections.synchronizedMap(new HashMap<>());
private final Map<Triple<String, TypeCapture<?>, Tags>, Object> cache = Collections.synchronizedMap(new HashMap<>());
private final Map<Triple<String, TypeCapture<?>, Tags>, GResultOf<Object>> cacheResultsOf = Collections.synchronizedMap(new HashMap<>());
private final Tags defaultTags;
private final ObservationService observationService;
private final GestaltConfig gestaltConfig;
Expand Down Expand Up @@ -66,7 +67,7 @@ public <T> T getConfig(String path, Class<T> klass) throws GestaltException {
Objects.requireNonNull(klass);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigInternal(path, typeCapture, null).results();
return getConfigInternal(path, typeCapture, null);
}

@Override
Expand All @@ -76,15 +77,15 @@ public <T> T getConfig(String path, Class<T> klass, Tags tags) throws GestaltExc
Objects.requireNonNull(tags);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigInternal(path, typeCapture, tags).results();
return getConfigInternal(path, typeCapture, tags);
}

@Override
public <T> T getConfig(String path, TypeCapture<T> klass) throws GestaltException {
Objects.requireNonNull(path);
Objects.requireNonNull(klass);

return getConfigInternal(path, klass, null).results();
return getConfigInternal(path, klass, null);
}

@Override
Expand All @@ -93,7 +94,7 @@ public <T> T getConfig(String path, TypeCapture<T> klass, Tags tags) throws Gest
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigInternal(path, klass, tags).results();
return getConfigInternal(path, klass, tags);
}

@Override
Expand All @@ -102,30 +103,54 @@ public <T> GResultOf<T> getConfigResult(String path, TypeCapture<T> klass, Tags
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigInternal(path, klass, tags);
return getConfigInternalResult(path, klass, tags);
}

@SuppressWarnings("unchecked")
private <T> GResultOf<T> getConfigInternal(String path, TypeCapture<T> klass, Tags tags) throws GestaltException {
private <T> T getConfigInternal(String path, TypeCapture<T> klass, Tags tags) throws GestaltException {

Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
if (cache.get(key) != null && cache.get(key).hasResults()) {
if (cache.get(key) != null) {
if (gestaltConfig.isObservationsEnabled() && observationService != null) {
observationService.recordObservation("cache.hit", 1, Tags.of());
}
return (GResultOf<T>) cache.get(key);
return (T) cache.get(key);
} else {
GResultOf<T> result = delegate.getConfigResult(path, klass, resolvedTags);
updateCache(path, key, result);
return result != null ? result.results(): null;
}
}

@SuppressWarnings("unchecked")
private <T> GResultOf<T> getConfigInternalResult(String path, TypeCapture<T> klass, Tags tags) throws GestaltException {

Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
if (cacheResultsOf.get(key) != null && cacheResultsOf.get(key).hasResults()) {
if (gestaltConfig.isObservationsEnabled() && observationService != null) {
observationService.recordObservation("cache.hit", 1, Tags.of());
}
return (GResultOf<T>) cacheResultsOf.get(key);
} else {
GResultOf<T> result = delegate.getConfigResult(path, klass, resolvedTags);
updateCacheResults(path, key, result);
return result;
}
}

@SuppressWarnings("unchecked")
private <T> void updateCache(String path, Triple<String, TypeCapture<?>, Tags> key, GResultOf<T> result) {
if (shouldCacheValue(path, result != null ? result.getMetadata() : Map.of())) {
cache.put(key, (GResultOf<Object>) result);
cache.put(key, result != null ? result.results() : null);
}
}

@SuppressWarnings("unchecked")
private <T> void updateCacheResults(String path, Triple<String, TypeCapture<?>, Tags> key, GResultOf<T> result) {
if (shouldCacheValue(path, result != null ? result.getMetadata() : Map.of())) {
cacheResultsOf.put(key, (GResultOf<Object>) result);
}
}

Expand All @@ -135,7 +160,7 @@ public <T> T getConfig(String path, T defaultVal, Class<T> klass) {
Objects.requireNonNull(klass);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigInternal(path, defaultVal, typeCapture, null).results();
return getConfigInternal(path, defaultVal, typeCapture, null);
}

@Override
Expand All @@ -145,7 +170,7 @@ public <T> T getConfig(String path, T defaultVal, Class<T> klass, Tags tags) {
Objects.requireNonNull(tags);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigInternal(path, defaultVal, typeCapture, tags).results();
return getConfigInternal(path, defaultVal, typeCapture, tags);
}

@Override
Expand All @@ -154,7 +179,7 @@ public <T> GResultOf<T> getConfigResult(String path, T defaultVal, TypeCapture<T
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigInternal(path, defaultVal, klass, tags);
return getConfigInternalResult(path, defaultVal, klass, tags);
}


Expand All @@ -164,7 +189,7 @@ public <T> T getConfig(String path, T defaultVal, TypeCapture<T> klass) {
Objects.requireNonNull(path);
Objects.requireNonNull(klass);

return getConfigInternal(path, defaultVal, klass, null).results();
return getConfigInternal(path, defaultVal, klass, null);
}

@Override
Expand All @@ -173,15 +198,44 @@ public <T> T getConfig(String path, T defaultVal, TypeCapture<T> klass, Tags tag
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigInternal(path, defaultVal, klass, tags).results();
return getConfigInternal(path, defaultVal, klass, tags);
}

@SuppressWarnings("unchecked")
private <T> GResultOf<T> getConfigInternal(String path, T defaultVal, TypeCapture<T> klass, Tags tags) {
private <T> T getConfigInternal(String path, T defaultVal, TypeCapture<T> klass, Tags tags) {
Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
if (cache.containsKey(key)) {
GResultOf<T> result = (GResultOf<T>) cache.get(key);
T result = (T) cache.get(key);
if (result == null) {
result = defaultVal;
}

if (gestaltConfig.isObservationsEnabled() && observationService != null) {
observationService.recordObservation("cache.hit", 1, Tags.of());
}

return result;

} else {
Optional<GResultOf<T>> result = delegate.getConfigOptionalResult(path, klass, resolvedTags);

updateCache(path, key, result.orElse(null));

if (result.isPresent() && result.get().hasResults()) {
return result.get().results();
} else {
return defaultVal;
}
}
}

@SuppressWarnings("unchecked")
private <T> GResultOf<T> getConfigInternalResult(String path, T defaultVal, TypeCapture<T> klass, Tags tags) {
Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
if (cacheResultsOf.containsKey(key)) {
GResultOf<T> result = (GResultOf<T>) cacheResultsOf.get(key);
if (result == null || !result.hasResults()) {
result = GResultOf.result(defaultVal, true);
}
Expand All @@ -195,7 +249,7 @@ private <T> GResultOf<T> getConfigInternal(String path, T defaultVal, TypeCaptur
} else {
Optional<GResultOf<T>> result = delegate.getConfigOptionalResult(path, klass, resolvedTags);

updateCache(path, key, result.orElse(null));
updateCacheResults(path, key, result.orElse(null));

if (result.isPresent() && result.get().hasResults()) {
return result.get();
Expand All @@ -211,7 +265,7 @@ public <T> Optional<T> getConfigOptional(String path, Class<T> klass) {
Objects.requireNonNull(klass);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigOptionalInternal(path, typeCapture, null).map(GResultOf::results);
return getConfigOptionalInternal(path, typeCapture, null);
}

@Override
Expand All @@ -221,15 +275,15 @@ public <T> Optional<T> getConfigOptional(String path, Class<T> klass, Tags tags)
Objects.requireNonNull(tags);

TypeCapture<T> typeCapture = TypeCapture.of(klass);
return getConfigOptionalInternal(path, typeCapture, tags).map(GResultOf::results);
return getConfigOptionalInternal(path, typeCapture, tags);
}

@Override
public <T> Optional<T> getConfigOptional(String path, TypeCapture<T> klass) {
Objects.requireNonNull(path);
Objects.requireNonNull(klass);

return getConfigOptionalInternal(path, klass, null).map(GResultOf::results);
return getConfigOptionalInternal(path, klass, null);
}

@Override
Expand All @@ -238,7 +292,7 @@ public <T> Optional<T> getConfigOptional(String path, TypeCapture<T> klass, Tags
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigOptionalInternal(path, klass, tags).map(GResultOf::results);
return getConfigOptionalInternal(path, klass, tags);
}

@Override
Expand All @@ -247,11 +301,11 @@ public <T> Optional<GResultOf<T>> getConfigOptionalResult(String path, TypeCaptu
Objects.requireNonNull(klass);
Objects.requireNonNull(tags);

return getConfigOptionalInternal(path, klass, tags);
return getConfigOptionalInternalResult(path, klass, tags);
}

@SuppressWarnings("unchecked")
public <T> Optional<GResultOf<T>> getConfigOptionalInternal(String path, TypeCapture<T> klass, Tags tags) {
public <T> Optional<T> getConfigOptionalInternal(String path, TypeCapture<T> klass, Tags tags) {

Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
Expand All @@ -260,12 +314,31 @@ public <T> Optional<GResultOf<T>> getConfigOptionalInternal(String path, TypeCap
observationService.recordObservation("cache.hit", 1, Tags.of());
}

GResultOf<T> result = (GResultOf<T>) cache.get(key);
T result = (T) cache.get(key);
return Optional.ofNullable(result);
} else {
Optional<GResultOf<T>> resultOptional = delegate.getConfigOptionalResult(path, klass, resolvedTags);
GResultOf<T> result = resultOptional.orElse(null);
updateCache(path, key, result);
return Optional.ofNullable(result != null ? result.results() : null);
}
}

public <T> Optional<GResultOf<T>> getConfigOptionalInternalResult(String path, TypeCapture<T> klass, Tags tags) {

Tags resolvedTags = tagMergingStrategy.mergeTags(tags, defaultTags);
Triple<String, TypeCapture<?>, Tags> key = new Triple<>(path, klass, resolvedTags);
if (cacheResultsOf.containsKey(key)) {
if (gestaltConfig.isObservationsEnabled() && observationService != null) {
observationService.recordObservation("cache.hit", 1, Tags.of());
}

GResultOf<T> result = (GResultOf<T>) cacheResultsOf.get(key);
return Optional.ofNullable(result);
} else {
Optional<GResultOf<T>> resultOptional = delegate.getConfigOptionalResult(path, klass, resolvedTags);
GResultOf<T> result = resultOptional.orElse(null);
updateCacheResults(path, key, result);
return Optional.ofNullable(result);
}
}
Expand All @@ -274,9 +347,9 @@ private boolean shouldCacheValue(String path, Map<String, List<MetaDataValue<?>>
boolean notIsSecret = nonCacheableSecrets.stream().noneMatch(it -> it.isSecret(path));
boolean noCacheMetadata = metadata.containsKey(IsNoCacheMetadata.NO_CACHE) &&
metadata.get(IsNoCacheMetadata.NO_CACHE).stream()
.map(it -> it instanceof IsNoCacheMetadata && ((IsNoCacheMetadata) it).getMetadata())
.filter(it -> it)
.findFirst().orElse(false);
.map(it -> it instanceof IsNoCacheMetadata && ((IsNoCacheMetadata) it).getMetadata())
.filter(it -> it)
.findFirst().orElse(false);

return notIsSecret && !noCacheMetadata;
}
Expand Down
2 changes: 1 addition & 1 deletion gestalt-core/src/test/java/org/github/gestalt/config/GestaltCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ void getConfig() throws GestaltException {
Assertions.assertEquals(100, port6.results());
Assertions.assertEquals(100, port7.get().results());

Mockito.verify(mockGestalt, Mockito.times(1)).getConfigResult("db.port", TypeCapture.of(Integer.class), Tags.of());
Mockito.verify(mockGestalt, Mockito.times(2)).getConfigResult("db.port", TypeCapture.of(Integer.class), Tags.of());
}

@Test
Expand Down

0 comments on commit 706c1cd

Please sign in to comment.