Skip to content

Commit c0f5565

Browse files
jed326Jay Deng
authored andcommitted
Use cached SourceLookup for FieldScript
Signed-off-by: Jay Deng <jayd0104@gmail.com>
1 parent d3eb8fe commit c0f5565

File tree

2 files changed

+27
-5
lines changed

2 files changed

+27
-5
lines changed

server/src/main/java/org/opensearch/search/lookup/SearchLookup.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import java.util.LinkedHashSet;
4343
import java.util.Objects;
4444
import java.util.Set;
45+
import java.util.concurrent.ConcurrentHashMap;
4546
import java.util.function.BiFunction;
4647
import java.util.function.Supplier;
4748

@@ -77,10 +78,10 @@ public class SearchLookup {
7778
*/
7879
private final Set<String> fieldChain;
7980
private final DocLookup docMap;
80-
private final SourceLookup sourceLookup;
8181
private final FieldsLookup fieldsLookup;
8282
private final BiFunction<MappedFieldType, Supplier<SearchLookup>, IndexFieldData<?>> fieldDataLookup;
8383
private final int shardId;
84+
private final ConcurrentHashMap<Long, SourceLookup> sourceLookupMap = new ConcurrentHashMap<>();
8485

8586
/**
8687
* Constructor for backwards compatibility. Use the one with explicit shardId argument.
@@ -107,7 +108,6 @@ public SearchLookup(
107108
mapperService,
108109
fieldType -> fieldDataLookup.apply(fieldType, () -> forkAndTrackFieldReferences(fieldType.name()))
109110
);
110-
sourceLookup = new SourceLookup();
111111
fieldsLookup = new FieldsLookup(mapperService);
112112
this.fieldDataLookup = fieldDataLookup;
113113
this.shardId = shardId;
@@ -126,7 +126,6 @@ private SearchLookup(SearchLookup searchLookup, Set<String> fieldChain) {
126126
searchLookup.docMap.mapperService(),
127127
fieldType -> searchLookup.fieldDataLookup.apply(fieldType, () -> forkAndTrackFieldReferences(fieldType.name()))
128128
);
129-
this.sourceLookup = searchLookup.sourceLookup;
130129
this.fieldsLookup = searchLookup.fieldsLookup;
131130
this.fieldDataLookup = searchLookup.fieldDataLookup;
132131
this.shardId = searchLookup.shardId;
@@ -160,7 +159,7 @@ public LeafSearchLookup getLeafSearchLookup(LeafReaderContext context) {
160159
return new LeafSearchLookup(
161160
context,
162161
docMap.getLeafDocLookup(context),
163-
new SourceLookup(),
162+
sourceLookupMap.computeIfAbsent(Thread.currentThread().threadId(), K -> new SourceLookup()),
164163
fieldsLookup.getLeafFieldsLookup(context)
165164
);
166165
}
@@ -173,7 +172,7 @@ public DocLookup doc() {
173172
* Returned SourceLookup will be unrelated to any created LeafSearchLookups. Instead, use {@link LeafSearchLookup#source()} to access the related {@link SearchLookup}.
174173
*/
175174
public SourceLookup source() {
176-
return sourceLookup;
175+
return sourceLookupMap.computeIfAbsent(Thread.currentThread().threadId(), K -> new SourceLookup());
177176
}
178177

179178
public int shardId() {

server/src/test/java/org/opensearch/script/ScriptServiceTests.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
package org.opensearch.script;
3333

34+
import org.apache.lucene.index.LeafReaderContext;
3435
import org.opensearch.ResourceNotFoundException;
3536
import org.opensearch.action.admin.cluster.storedscripts.GetStoredScriptRequest;
3637
import org.opensearch.cluster.ClusterName;
@@ -46,6 +47,8 @@
4647
import org.opensearch.core.common.bytes.BytesReference;
4748
import org.opensearch.core.xcontent.MediaTypeRegistry;
4849
import org.opensearch.env.Environment;
50+
import org.opensearch.index.mapper.MapperService;
51+
import org.opensearch.search.lookup.SearchLookup;
4952
import org.opensearch.test.OpenSearchTestCase;
5053
import org.junit.Before;
5154

@@ -67,6 +70,7 @@
6770
import static org.hamcrest.Matchers.is;
6871
import static org.hamcrest.Matchers.notNullValue;
6972
import static org.hamcrest.Matchers.sameInstance;
73+
import static org.mockito.Mockito.mock;
7074

7175
public class ScriptServiceTests extends OpenSearchTestCase {
7276

@@ -180,6 +184,25 @@ public void testInlineScriptCompiledOnceCache() throws IOException {
180184
assertThat(factoryScript1, sameInstance(factoryScript2));
181185
}
182186

187+
public void testScriptsUseCachedSourceLookup() throws IOException {
188+
buildScriptService(Settings.EMPTY);
189+
Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap());
190+
FieldScript.Factory factoryScript1 = scriptService.compile(script, FieldScript.CONTEXT);
191+
FieldScript.LeafFactory leafFactory1 = factoryScript1.newFactory(
192+
new HashMap<>(),
193+
new SearchLookup(mock(MapperService.class), null)
194+
);
195+
196+
LeafReaderContext ctx = mock(LeafReaderContext.class);
197+
FieldScript script1 = leafFactory1.newInstance(ctx);
198+
FieldScript script2 = leafFactory1.newInstance(ctx);
199+
200+
script1.setDocument(1);
201+
script2.setDocument(1);
202+
203+
assertThat(script1.getLeafLookup().source(), sameInstance(script2.getLeafLookup().source()));
204+
}
205+
183206
public void testAllowAllScriptTypeSettings() throws IOException {
184207
buildScriptService(Settings.EMPTY);
185208

0 commit comments

Comments
 (0)