Merge pull request #17785 from vespa-engine/balder/reduce-lifetime-of-potentially-large-objects

Reduce the lifetime and scope of ConfigInstance.Builder and the Confi…

6 files changed, 39 insertions, 33 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/SuperModelController.java b/configserver/src/main/java/com/yahoo/vespa/config/server/SuperModelController.java
index 0bddd8d0637..d79d03bc8c5 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/SuperModelController.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/SuperModelController.java
@@ -45,8 +45,7 @@ public class SuperModelController {
     public ConfigResponse resolveConfig(GetConfigRequest request) {
         ConfigKey<?> configKey = request.getConfigKey();
         validateConfigDefinition(request.getConfigKey(), request.getDefContent());
-        ConfigPayload payload = model.getConfig(configKey);
-        return responseFactory.createResponse(payload, generation, false);
+        return responseFactory.createResponse(model.getConfig(configKey).toUtf8Array(true), generation, false);
     }
 
     private void validateConfigDefinition(ConfigKey<?> configKey, DefContent defContent) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
index baded957475..dfe06149ddf 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.application;
 
+import com.yahoo.collections.Pair;
 import com.yahoo.component.Version;
 import com.yahoo.config.ConfigInstance;
 import com.yahoo.config.ConfigurationRuntimeException;
@@ -8,6 +9,8 @@ import com.yahoo.config.model.api.ApplicationInfo;
 import com.yahoo.config.model.api.Model;
 import com.yahoo.config.provision.ApplicationId;
 import java.util.logging.Level;
+
+import com.yahoo.text.Utf8Array;
 import com.yahoo.vespa.config.ConfigCacheKey;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.ConfigKey;
@@ -123,45 +126,52 @@ public class Application implements ModelResult {
         }
         log.log(Level.FINE, () -> TenantRepository.logPre(getId()) + ("Resolving " + configKey + " with config definition " + def));
 
-        ConfigInstance.Builder builder;
-        ConfigPayload payload;
-        boolean applyOnRestart = false;
+
+
+        var payload = createPayload(configKey, def);
+        ConfigResponse configResponse = responseFactory.createResponse(payload.getFirst(),
+                                                                       applicationGeneration,
+                                                                       payload.getSecond());
+        metricUpdater.incrementProcTime(System.currentTimeMillis() - start);
+        if (useCache(req)) {
+            cache.put(cacheKey, configResponse, configResponse.getConfigMd5());
+            metricUpdater.setCacheConfigElems(cache.configElems());
+            metricUpdater.setCacheChecksumElems(cache.checkSumElems());
+        }
+        return configResponse;
+    }
+
+    private Pair<Utf8Array, Boolean> createPayload(ConfigKey<?> configKey, ConfigDefinition def) {
         try {
-            builder = model.getConfigInstance(configKey, def);
+            ConfigInstance.Builder builder = model.getConfigInstance(configKey, def);
+            boolean tempApplyOnRestart = builder.getApplyOnRestart();
             if (builder instanceof GenericConfig.GenericConfigBuilder) {
-                payload = ((GenericConfig.GenericConfigBuilder) builder).getPayload();
-                applyOnRestart = builder.getApplyOnRestart();
+                return new Pair<>(((GenericConfig.GenericConfigBuilder) builder).getPayload().toUtf8Array(true),
+                        tempApplyOnRestart);
             }
             else {
+                String cacheBuilderClassNameForErrorReport = builder.getClass().getName();
+                ConfigPayload payload;
+                boolean applyOnRestart = false;
                 try {
                     ConfigInstance instance = ConfigInstanceBuilder.buildInstance(builder, def.getCNode());
                     payload = ConfigPayload.fromInstance(instance);
-                    applyOnRestart = builder.getApplyOnRestart();
+                    applyOnRestart = tempApplyOnRestart;
                 } catch (ConfigurationRuntimeException e) {
                     // This can happen in cases where services ask for config that no longer exist before they have been able
                     // to reconfigure themselves
                     log.log(Level.INFO, TenantRepository.logPre(getId()) +
-                                        ": Error resolving instance for builder '" + builder.getClass().getName() +
-                                        "', returning empty config: " + Exceptions.toMessageString(e));
+                            ": Error resolving instance for builder '" + cacheBuilderClassNameForErrorReport +
+                            "', returning empty config: " + Exceptions.toMessageString(e));
                     payload = ConfigPayload.fromBuilder(new ConfigPayloadBuilder());
                 }
                 if (def.getCNode() != null)
                     payload.applyDefaultsFromDef(def.getCNode());
+                return new Pair<>(payload.toUtf8Array(true), applyOnRestart);
             }
         } catch (Exception e) {
             throw new ConfigurationRuntimeException("Unable to get config for " + app, e);
         }
-
-        ConfigResponse configResponse = responseFactory.createResponse(payload,
-                                                                       applicationGeneration,
-                                                                       applyOnRestart);
-        metricUpdater.incrementProcTime(System.currentTimeMillis() - start);
-        if (useCache(req)) {
-            cache.put(cacheKey, configResponse, configResponse.getConfigMd5());
-            metricUpdater.setCacheConfigElems(cache.configElems());
-            metricUpdater.setCacheChecksumElems(cache.checkSumElems());
-        }
-        return configResponse;
     }
 
     private boolean useCache(GetConfigRequest request) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactory.java
index 8003b2a2be9..3811e61c5a9 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactory.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactory.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.config.server.rpc;
 
 import com.yahoo.cloud.config.ConfigserverConfig;
+import com.yahoo.text.Utf8Array;
 import com.yahoo.vespa.config.ConfigPayload;
 import com.yahoo.vespa.config.protocol.ConfigResponse;
 
@@ -27,12 +28,12 @@ public interface ConfigResponseFactory {
     /**
      * Creates a {@link ConfigResponse} for a given payload and generation.
      *
-     * @param payload        the {@link ConfigPayload} to put in the response
+     * @param rawPayload     the {@link ConfigPayload} to put in the response
      * @param generation     the payload generation
      * @param applyOnRestart true if this config change should only be applied on restart,
      *                       false if it should be applied immediately
      * @return a {@link ConfigResponse} that can be sent to the client
      */
-    ConfigResponse createResponse(ConfigPayload payload, long generation, boolean applyOnRestart);
+    ConfigResponse createResponse(Utf8Array rawPayload, long generation, boolean applyOnRestart);
 
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/LZ4ConfigResponseFactory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/LZ4ConfigResponseFactory.java
index 619e6c0a2a2..be6964003bc 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/LZ4ConfigResponseFactory.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/LZ4ConfigResponseFactory.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.config.server.rpc;
 
 import com.yahoo.text.Utf8Array;
-import com.yahoo.vespa.config.ConfigPayload;
 import com.yahoo.vespa.config.LZ4PayloadCompressor;
 import com.yahoo.vespa.config.protocol.CompressionInfo;
 import com.yahoo.vespa.config.protocol.CompressionType;
@@ -20,13 +19,12 @@ public class LZ4ConfigResponseFactory implements ConfigResponseFactory {
     private static final LZ4PayloadCompressor compressor = new LZ4PayloadCompressor();
 
     @Override
-    public ConfigResponse createResponse(ConfigPayload payload,
+    public ConfigResponse createResponse(Utf8Array rawPayload,
                                          long generation,
                                          boolean applyOnRestart) {
-        Utf8Array rawPayload = payload.toUtf8Array(true);
         String configMd5 = ConfigUtils.getMd5(rawPayload);
         CompressionInfo info = CompressionInfo.create(CompressionType.LZ4, rawPayload.getByteLength());
-        Utf8Array compressed = new Utf8Array(compressor.compress(rawPayload.getBytes()));
+        Utf8Array compressed = new Utf8Array(compressor.compress(rawPayload.wrap()));
         return new SlimeConfigResponse(compressed, generation, applyOnRestart, configMd5, info);
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/UncompressedConfigResponseFactory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/UncompressedConfigResponseFactory.java
index 8d852ebd8c9..709e3874bc7 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/UncompressedConfigResponseFactory.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/UncompressedConfigResponseFactory.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.config.server.rpc;
 
 import com.yahoo.text.Utf8Array;
-import com.yahoo.vespa.config.ConfigPayload;
 import com.yahoo.vespa.config.protocol.CompressionInfo;
 import com.yahoo.vespa.config.protocol.CompressionType;
 import com.yahoo.vespa.config.protocol.ConfigResponse;
@@ -17,10 +16,9 @@ import com.yahoo.vespa.config.util.ConfigUtils;
 public class UncompressedConfigResponseFactory implements ConfigResponseFactory {
 
     @Override
-    public ConfigResponse createResponse(ConfigPayload payload,
+    public ConfigResponse createResponse(Utf8Array rawPayload,
                                          long generation,
                                          boolean applyOnRestart) {
-        Utf8Array rawPayload = payload.toUtf8Array(true);
         String configMd5 = ConfigUtils.getMd5(rawPayload);
         CompressionInfo info = CompressionInfo.create(CompressionType.UNCOMPRESSED, rawPayload.getByteLength());
         return new SlimeConfigResponse(rawPayload, generation, applyOnRestart, configMd5, info);
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactoryTest.java
index 5d8b8c92472..747a0ad3241 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/ConfigResponseFactoryTest.java
@@ -16,7 +16,7 @@ public class ConfigResponseFactoryTest {
     @Test
     public void testUncompressedFactory() {
         UncompressedConfigResponseFactory responseFactory = new UncompressedConfigResponseFactory();
-        ConfigResponse response = responseFactory.createResponse(ConfigPayload.empty(), 3, false);
+        ConfigResponse response = responseFactory.createResponse(ConfigPayload.empty().toUtf8Array(true), 3, false);
         assertEquals(CompressionType.UNCOMPRESSED, response.getCompressionInfo().getCompressionType());
         assertEquals(3L,response.getGeneration());
         assertEquals(2, response.getPayload().getByteLength());
@@ -25,7 +25,7 @@ public class ConfigResponseFactoryTest {
     @Test
     public void testLZ4CompressedFactory() {
         LZ4ConfigResponseFactory responseFactory = new LZ4ConfigResponseFactory();
-        ConfigResponse response = responseFactory.createResponse(ConfigPayload.empty(), 3, false);
+        ConfigResponse response = responseFactory.createResponse(ConfigPayload.empty().toUtf8Array(true), 3, false);
         assertEquals(CompressionType.LZ4, response.getCompressionInfo().getCompressionType());
         assertEquals(3L, response.getGeneration());
         assertEquals(3, response.getPayload().getByteLength());