From f8d4b0c1d5dab78921ef0b219b815489d82e9d3d Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 20 Mar 2020 13:29:34 -0600 Subject: [PATCH] Scripting: Context script cache unlimited compile (#53769) * Adds "unlimited" compilation rate for context script caches * `script.context.${CONTEXT}.max_compilations_rate` = `unlimited` disables compilation rate limiting for `${CONTEXT}`'s script cache Refs: #50152 --- .../org/elasticsearch/script/ScriptCache.java | 11 ++++---- .../elasticsearch/script/ScriptService.java | 8 +++++- .../script/ScriptCacheTests.java | 13 ++++++++++ .../script/ScriptServiceTests.java | 25 +++++++++++++++++-- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index a37851cc81050..7ab5f6d131718 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -40,14 +40,16 @@ public class ScriptCache { private static final Logger logger = LogManager.getLogger(ScriptService.class); + static final Tuple UNLIMITED_COMPILATION_RATE = new Tuple<>(0, TimeValue.ZERO); + private final Cache cache; private final ScriptMetrics scriptMetrics; private final Object lock = new Object(); - // Mutable fields - private long lastInlineCompileTime; - private double scriptsPerTimeWindow; + // Mutable fields, visible for tests + long lastInlineCompileTime; + double scriptsPerTimeWindow; // Cache settings or derived from settings final int cacheSize; @@ -150,8 +152,7 @@ public ScriptStats stats() { * is discarded - there can never be more water in the bucket than the size of the bucket. */ void checkCompilationLimit() { - if (rate.v1() == 0 && rate.v2().getNanos() == 0) { - // unlimited + if (rate.equals(UNLIMITED_COMPILATION_RATE)) { return; } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index 66ed3fee0d6ca..5f8edfda3a278 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -128,10 +128,16 @@ public class ScriptService implements Closeable, ClusterStateApplier { key -> Setting.positiveTimeSetting(key, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, TimeValue.timeValueMillis(0), Property.NodeScope, Property.Dynamic)); + // Unlimited compilation rate for context-specific script caches + static final String UNLIMITED_COMPILATION_RATE_KEY = "unlimited"; + public static final Setting.AffixSetting> SCRIPT_MAX_COMPILATIONS_RATE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "max_compilations_rate", - key -> new Setting<>(key, "75/5m", MAX_COMPILATION_RATE_FUNCTION, Property.NodeScope, Property.Dynamic)); + key -> new Setting<>(key, "75/5m", + (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: + MAX_COMPILATION_RATE_FUNCTION.apply(value), + Property.NodeScope, Property.Dynamic)); private static final Tuple SCRIPT_COMPILATION_RATE_ZERO = new Tuple<>(0, TimeValue.ZERO); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index dfa946099306f..96c724b5720c6 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -52,4 +52,17 @@ public void testCompilationCircuitBreaking() throws Exception { cache.checkCompilationLimit(); } } + + public void testUnlimitedCompilationRate() { + final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY); + final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY); + ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE); + long lastInlineCompileTime = cache.lastInlineCompileTime; + double scriptsPerTimeWindow = cache.scriptsPerTimeWindow; + for(int i=0; i < 3000; i++) { + cache.checkCompilationLimit(); + assertEquals(lastInlineCompileTime, cache.lastInlineCompileTime); + assertEquals(scriptsPerTimeWindow, cache.scriptsPerTimeWindow, 0.0); // delta of 0.0 because it should never change + } + } } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 77ce3fe127c7f..4130c723dbe33 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -450,6 +450,24 @@ public void testCacheHolderContextConstructor() { assertEquals(zero, holder.contextCache.get("baz").get().rate); } + public void testCompilationRateUnlimitedContextOnly() throws IOException { + IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .build()); + }); + assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [unlimited]", illegal.getMessage()); + + // Should not throw. + buildScriptService(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("field").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) + .build()); + } + public void testCacheHolderChangeSettings() { String fooCompilationRate = "77/5m"; String barCompilationRate = "78/6m"; @@ -459,7 +477,7 @@ public void testCacheHolderChangeSettings() { Settings s = Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) .build(); - Set contexts = Set.of("foo", "bar", "baz"); + Set contexts = Set.of("foo", "bar", "baz", "qux"); ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, true); assertNotNull(holder.general); @@ -470,16 +488,19 @@ public void testCacheHolderChangeSettings() { .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), fooCompilationRate) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), barCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("qux").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) .build() ); assertNull(holder.general); assertNotNull(holder.contextCache); - assertEquals(3, holder.contextCache.size()); + assertEquals(4, holder.contextCache.size()); assertEquals(contexts, holder.contextCache.keySet()); assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("foo").get().rate); assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.contextCache.get("bar").get().rate); + assertEquals(ScriptCache.UNLIMITED_COMPILATION_RATE, holder.contextCache.get("qux").get().rate); assertEquals(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getDefault(Settings.EMPTY), holder.contextCache.get("baz").get().rate);