Skip to content

Commit 9a9bc69

Browse files
iveraseiverase
authored
Stop caching source map on SearchHit#getSourceMap (#119888)
This call has the side effect that if you are iterating a number of hits calling this method, you will be increasing the memory usage by a non trivial number which in most of cases is unwanted. Therefore this commit removes this caching all together and add an assertion so the method is call once during the lifetime of the object.
1 parent 787a16d commit 9a9bc69

File tree

55 files changed

+461
-354
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

55 files changed

+461
-354
lines changed

‎modules/parent-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,9 @@ public void testSimpleChildQuery() throws Exception {
184184
assertNoFailuresAndResponse(prepareSearch("test").setQuery(idsQuery().addIds("c1")), response -> {
185185
assertThat(response.getHits().getTotalHits().value(), equalTo(1L));
186186
assertThat(response.getHits().getAt(0).getId(), equalTo("c1"));
187-
assertThat(extractValue("join_field.name", response.getHits().getAt(0).getSourceAsMap()), equalTo("child"));
188-
assertThat(extractValue("join_field.parent", response.getHits().getAt(0).getSourceAsMap()), equalTo("p1"));
187+
Map<String, Object> source = response.getHits().getAt(0).getSourceAsMap();
188+
assertThat(extractValue("join_field.name", source), equalTo("child"));
189+
assertThat(extractValue("join_field.parent", source), equalTo("p1"));
189190

190191
});
191192

@@ -197,11 +198,13 @@ public void testSimpleChildQuery() throws Exception {
197198
response -> {
198199
assertThat(response.getHits().getTotalHits().value(), equalTo(2L));
199200
assertThat(response.getHits().getAt(0).getId(), anyOf(equalTo("c1"), equalTo("c2")));
200-
assertThat(extractValue("join_field.name", response.getHits().getAt(0).getSourceAsMap()), equalTo("child"));
201-
assertThat(extractValue("join_field.parent", response.getHits().getAt(0).getSourceAsMap()), equalTo("p1"));
201+
Map<String, Object> source0 = response.getHits().getAt(0).getSourceAsMap();
202+
assertThat(extractValue("join_field.name", source0), equalTo("child"));
203+
assertThat(extractValue("join_field.parent", source0), equalTo("p1"));
202204
assertThat(response.getHits().getAt(1).getId(), anyOf(equalTo("c1"), equalTo("c2")));
203-
assertThat(extractValue("join_field.name", response.getHits().getAt(1).getSourceAsMap()), equalTo("child"));
204-
assertThat(extractValue("join_field.parent", response.getHits().getAt(1).getSourceAsMap()), equalTo("p1"));
205+
Map<String, Object> source1 = response.getHits().getAt(1).getSourceAsMap();
206+
assertThat(extractValue("join_field.name", source1), equalTo("child"));
207+
assertThat(extractValue("join_field.parent", source1), equalTo("p1"));
205208
}
206209
);
207210

‎server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -655,8 +655,9 @@ public void testFetchFeatures() throws IOException {
655655
assertThat(hit.field("field2").getValue(), equalTo(2.71f));
656656
assertThat(hit.field("script").getValue().toString(), equalTo("5"));
657657

658-
assertThat(hit.getSourceAsMap().size(), equalTo(1));
659-
assertThat(hit.getSourceAsMap().get("text").toString(), equalTo("some text to entertain"));
658+
Map<String, Object> source = hit.getSourceAsMap();
659+
assertThat(source.size(), equalTo(1));
660+
assertThat(source.get("text").toString(), equalTo("some text to entertain"));
660661
assertEquals("some text to entertain", hit.getFields().get("text").getValue());
661662
assertEquals("some text to entertain", hit.getFields().get("text_stored_lookup").getValue());
662663
}
@@ -927,8 +928,9 @@ public void testNestedFetchFeatures() {
927928
field = searchHit.field("script");
928929
assertThat(field.getValue().toString(), equalTo("5"));
929930

930-
assertThat(searchHit.getSourceAsMap().size(), equalTo(1));
931-
assertThat(extractValue("message", searchHit.getSourceAsMap()), equalTo("some comment"));
931+
Map<String, Object> source = searchHit.getSourceAsMap();
932+
assertThat(source.size(), equalTo(1));
933+
assertThat(extractValue("message", source), equalTo("some comment"));
932934
}
933935
);
934936
}

‎server/src/internalClusterTest/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java‎

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -490,8 +490,9 @@ public void testNestedMultipleLayers() throws Exception {
490490
response -> {
491491
SearchHits innerHits = response.getHits().getAt(0).getInnerHits().get("comments");
492492
innerHits = innerHits.getAt(0).getInnerHits().get("remark");
493-
assertNotNull(innerHits.getAt(0).getSourceAsMap());
494-
assertFalse(innerHits.getAt(0).getSourceAsMap().isEmpty());
493+
Map<String, Object> source = innerHits.getAt(0).getSourceAsMap();
494+
assertNotNull(source);
495+
assertFalse(source.isEmpty());
495496
}
496497
);
497498
assertNoFailuresAndResponse(
@@ -507,8 +508,9 @@ public void testNestedMultipleLayers() throws Exception {
507508
response -> {
508509
SearchHits innerHits = response.getHits().getAt(0).getInnerHits().get("comments");
509510
innerHits = innerHits.getAt(0).getInnerHits().get("remark");
510-
assertNotNull(innerHits.getAt(0).getSourceAsMap());
511-
assertFalse(innerHits.getAt(0).getSourceAsMap().isEmpty());
511+
Map<String, Object> source = innerHits.getAt(0).getSourceAsMap();
512+
assertNotNull(source);
513+
assertFalse(source.isEmpty());
512514
}
513515
);
514516
}
@@ -845,16 +847,12 @@ public void testNestedSource() throws Exception {
845847
assertHitCount(response, 1);
846848

847849
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getTotalHits().value(), equalTo(2L));
848-
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap().size(), equalTo(1));
849-
assertThat(
850-
response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap().get("message"),
851-
equalTo("fox eat quick")
852-
);
853-
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getAt(1).getSourceAsMap().size(), equalTo(1));
854-
assertThat(
855-
response.getHits().getAt(0).getInnerHits().get("comments").getAt(1).getSourceAsMap().get("message"),
856-
equalTo("fox ate rabbit x y z")
857-
);
850+
Map<String, Object> source0 = response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap();
851+
assertThat(source0.size(), equalTo(1));
852+
assertThat(source0.get("message"), equalTo("fox eat quick"));
853+
Map<String, Object> source1 = response.getHits().getAt(0).getInnerHits().get("comments").getAt(1).getSourceAsMap();
854+
assertThat(source1.size(), equalTo(1));
855+
assertThat(source1.get("message"), equalTo("fox ate rabbit x y z"));
858856
}
859857
);
860858

@@ -866,16 +864,12 @@ public void testNestedSource() throws Exception {
866864
assertHitCount(response, 1);
867865

868866
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getTotalHits().value(), equalTo(2L));
869-
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap().size(), equalTo(2));
870-
assertThat(
871-
response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap().get("message"),
872-
equalTo("fox eat quick")
873-
);
874-
assertThat(response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap().size(), equalTo(2));
875-
assertThat(
876-
response.getHits().getAt(0).getInnerHits().get("comments").getAt(1).getSourceAsMap().get("message"),
877-
equalTo("fox ate rabbit x y z")
878-
);
867+
Map<String, Object> source0 = response.getHits().getAt(0).getInnerHits().get("comments").getAt(0).getSourceAsMap();
868+
assertThat(source0.size(), equalTo(2));
869+
assertThat(source0.get("message"), equalTo("fox eat quick"));
870+
Map<String, Object> source1 = response.getHits().getAt(0).getInnerHits().get("comments").getAt(1).getSourceAsMap();
871+
assertThat(source1.size(), equalTo(2));
872+
assertThat(source1.get("message"), equalTo("fox ate rabbit x y z"));
879873
}
880874
);
881875

‎server/src/internalClusterTest/java/org/elasticsearch/search/searchafter/SearchAfterIT.java‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import java.util.Collections;
4848
import java.util.Comparator;
4949
import java.util.List;
50+
import java.util.Map;
5051

5152
import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
5253
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
@@ -152,8 +153,9 @@ public void testWithNullStrings() throws InterruptedException {
152153
searchResponse -> {
153154
assertThat(searchResponse.getHits().getTotalHits().value(), Matchers.equalTo(2L));
154155
assertThat(searchResponse.getHits().getHits().length, Matchers.equalTo(1));
155-
assertThat(searchResponse.getHits().getHits()[0].getSourceAsMap().get("field1"), Matchers.equalTo(100));
156-
assertThat(searchResponse.getHits().getHits()[0].getSourceAsMap().get("field2"), Matchers.equalTo("toto"));
156+
Map<String, Object> source = searchResponse.getHits().getHits()[0].getSourceAsMap();
157+
assertThat(source.get("field1"), Matchers.equalTo(100));
158+
assertThat(source.get("field2"), Matchers.equalTo("toto"));
157159
}
158160
);
159161
}
@@ -438,8 +440,9 @@ public void testScrollAndSearchAfterWithBigIndex() {
438440
int foundHits = 0;
439441
do {
440442
for (SearchHit hit : resp.getHits().getHits()) {
441-
assertNotNull(hit.getSourceAsMap());
442-
final Object timestamp = hit.getSourceAsMap().get("timestamp");
443+
Map<String, Object> source = hit.getSourceAsMap();
444+
assertNotNull(source);
445+
final Object timestamp = source.get("timestamp");
443446
assertNotNull(timestamp);
444447
assertThat(((Number) timestamp).longValue(), equalTo(timestamps.get(foundHits)));
445448
foundHits++;
@@ -469,8 +472,9 @@ public void testScrollAndSearchAfterWithBigIndex() {
469472
do {
470473
Object[] after = null;
471474
for (SearchHit hit : resp.getHits().getHits()) {
472-
assertNotNull(hit.getSourceAsMap());
473-
final Object timestamp = hit.getSourceAsMap().get("timestamp");
475+
Map<String, Object> source = hit.getSourceAsMap();
476+
assertNotNull(source);
477+
final Object timestamp = source.get("timestamp");
474478
assertNotNull(timestamp);
475479
assertThat(((Number) timestamp).longValue(), equalTo(timestamps.get(foundHits)));
476480
after = hit.getSortValues();
@@ -505,8 +509,9 @@ public void testScrollAndSearchAfterWithBigIndex() {
505509
do {
506510
Object[] after = null;
507511
for (SearchHit hit : resp.getHits().getHits()) {
508-
assertNotNull(hit.getSourceAsMap());
509-
final Object timestamp = hit.getSourceAsMap().get("timestamp");
512+
Map<String, Object> source = hit.getSourceAsMap();
513+
assertNotNull(source);
514+
final Object timestamp = source.get("timestamp");
510515
assertNotNull(timestamp);
511516
foundSeqNos.add(((Number) timestamp).longValue());
512517
after = hit.getSortValues();

‎server/src/internalClusterTest/java/org/elasticsearch/search/sort/FieldSortIT.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,11 @@ public void testIssue8226() {
130130
.setSize(10),
131131
response -> {
132132
logClusterState();
133+
Number previous = (Number) response.getHits().getHits()[0].getSourceAsMap().get("entry");
133134
for (int j = 1; j < response.getHits().getHits().length; j++) {
134135
Number current = (Number) response.getHits().getHits()[j].getSourceAsMap().get("entry");
135-
Number previous = (Number) response.getHits().getHits()[j - 1].getSourceAsMap().get("entry");
136136
assertThat(response.toString(), current.intValue(), lessThan(previous.intValue()));
137+
previous = current;
137138
}
138139
}
139140
);
@@ -144,10 +145,11 @@ public void testIssue8226() {
144145
.setSize(10),
145146
response -> {
146147
logClusterState();
148+
Number previous = (Number) response.getHits().getHits()[0].getSourceAsMap().get("entry");
147149
for (int j = 1; j < response.getHits().getHits().length; j++) {
148150
Number current = (Number) response.getHits().getHits()[j].getSourceAsMap().get("entry");
149-
Number previous = (Number) response.getHits().getHits()[j - 1].getSourceAsMap().get("entry");
150151
assertThat(response.toString(), current.intValue(), greaterThan(previous.intValue()));
152+
previous = current;
151153
}
152154
}
153155
);

‎server/src/internalClusterTest/java/org/elasticsearch/search/source/SourceFetchingIT.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
import org.elasticsearch.test.ESIntegTestCase;
1313

14+
import java.util.Map;
15+
1416
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
1517
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponses;
1618
import static org.hamcrest.Matchers.notNullValue;
@@ -57,8 +59,9 @@ public void testSourceFiltering() {
5759

5860
assertResponses(response -> {
5961
assertThat(response.getHits().getAt(0).getSourceAsString(), notNullValue());
60-
assertThat(response.getHits().getAt(0).getSourceAsMap().size(), equalTo(1));
61-
assertThat((String) response.getHits().getAt(0).getSourceAsMap().get("field1"), equalTo("value"));
62+
Map<String, Object> source = response.getHits().getAt(0).getSourceAsMap();
63+
assertThat(source.size(), equalTo(1));
64+
assertThat(source.get("field1"), equalTo("value"));
6265
},
6366
prepareSearch("test").setFetchSource("field1", null),
6467
prepareSearch("test").setFetchSource(new String[] { "*" }, new String[] { "field2" })
@@ -84,8 +87,9 @@ public void testSourceWithWildcardFiltering() {
8487

8588
assertResponses(response -> {
8689
assertThat(response.getHits().getAt(0).getSourceAsString(), notNullValue());
87-
assertThat(response.getHits().getAt(0).getSourceAsMap().size(), equalTo(1));
88-
assertThat((String) response.getHits().getAt(0).getSourceAsMap().get("field"), equalTo("value"));
90+
Map<String, Object> source = response.getHits().getAt(0).getSourceAsMap();
91+
assertThat(source.size(), equalTo(1));
92+
assertThat((String) source.get("field"), equalTo("value"));
8993
},
9094
prepareSearch("test").setFetchSource(new String[] { "*.notexisting", "field" }, null),
9195
prepareSearch("test").setFetchSource(new String[] { "field.notexisting.*", "field" }, null)

‎server/src/internalClusterTest/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -356,8 +356,9 @@ public void testSuggestDocumentSourceFiltering() throws Exception {
356356
assertThat(option.getText().toString(), equalTo("suggestion" + id));
357357
assertThat(option.getHit(), hasId("" + id));
358358
assertThat(option.getHit(), hasScore((id)));
359-
assertNotNull(option.getHit().getSourceAsMap());
360-
Set<String> sourceFields = option.getHit().getSourceAsMap().keySet();
359+
Map<String, Object> source = option.getHit().getSourceAsMap();
360+
assertNotNull(source);
361+
Set<String> sourceFields = source.keySet();
361362
assertThat(sourceFields, contains("a"));
362363
assertThat(sourceFields, not(contains("b")));
363364
id--;

‎server/src/main/java/org/elasticsearch/search/SearchHit.java‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ public final class SearchHit implements Writeable, ToXContentObject, RefCounted
104104
private transient String index;
105105
private transient String clusterAlias;
106106

107-
private Map<String, Object> sourceAsMap;
107+
// For asserting that the method #getSourceAsMap is called just once on the lifetime of this object
108+
private boolean sourceAsMapCalled = false;
108109

109110
private Map<String, SearchHits> innerHits;
110111

@@ -142,7 +143,6 @@ private SearchHit(int nestedTopDocId, String id, NestedIdentity nestedIdentity,
142143
null,
143144
null,
144145
null,
145-
null,
146146
new HashMap<>(),
147147
new HashMap<>(),
148148
refCounted
@@ -166,7 +166,6 @@ public SearchHit(
166166
SearchShardTarget shard,
167167
String index,
168168
String clusterAlias,
169-
Map<String, Object> sourceAsMap,
170169
Map<String, SearchHits> innerHits,
171170
Map<String, DocumentField> documentFields,
172171
Map<String, DocumentField> metaFields,
@@ -188,7 +187,6 @@ public SearchHit(
188187
this.shard = shard;
189188
this.index = index;
190189
this.clusterAlias = clusterAlias;
191-
this.sourceAsMap = sourceAsMap;
192190
this.innerHits = innerHits;
193191
this.documentFields = documentFields;
194192
this.metaFields = metaFields;
@@ -279,7 +277,6 @@ public static SearchHit readFrom(StreamInput in, boolean pooled) throws IOExcept
279277
shardTarget,
280278
index,
281279
clusterAlias,
282-
null,
283280
innerHits,
284281
documentFields,
285282
metaFields,
@@ -447,7 +444,6 @@ public BytesReference getSourceRef() {
447444
*/
448445
public SearchHit sourceRef(BytesReference source) {
449446
this.source = source;
450-
this.sourceAsMap = null;
451447
return this;
452448
}
453449

@@ -476,19 +472,18 @@ public String getSourceAsString() {
476472
}
477473

478474
/**
479-
* The source of the document as a map (can be {@code null}).
475+
* The source of the document as a map (can be {@code null}). This method is expected
476+
* to be called at most once during the lifetime of the object as the generated map
477+
* is expensive to generate and it does not get cache.
480478
*/
481479
public Map<String, Object> getSourceAsMap() {
482480
assert hasReferences();
481+
assert sourceAsMapCalled == false : "getSourceAsMap() called twice";
482+
sourceAsMapCalled = true;
483483
if (source == null) {
484484
return null;
485485
}
486-
if (sourceAsMap != null) {
487-
return sourceAsMap;
488-
}
489-
490-
sourceAsMap = Source.fromBytes(source).source();
491-
return sourceAsMap;
486+
return Source.fromBytes(source).source();
492487
}
493488

494489
/**
@@ -758,7 +753,6 @@ public SearchHit asUnpooled() {
758753
shard,
759754
index,
760755
clusterAlias,
761-
sourceAsMap,
762756
innerHits == null
763757
? null
764758
: innerHits.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().asUnpooled())),

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,8 @@ public Object getProperty(List<String> path) {
235235
} else if (tokens[0].equals(SCORE)) {
236236
return topHit.getScore();
237237
} else if (tokens[0].equals(SOURCE)) {
238+
// Caching the map might help here but memory usage is a concern for this class
239+
// This is dead code, pipeline aggregations do not support _source.field.
238240
Map<String, Object> sourceAsMap = topHit.getSourceAsMap();
239241
if (sourceAsMap != null) {
240242
Object property = sourceAsMap.get(tokens[1]);

‎server/src/test/java/org/elasticsearch/search/SearchHitTests.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,7 @@ public void testNullSource() {
317317

318318
assertThat(searchHit.getSourceAsMap(), nullValue());
319319
assertThat(searchHit.getSourceRef(), nullValue());
320-
assertThat(searchHit.getSourceAsMap(), nullValue());
321320
assertThat(searchHit.getSourceAsString(), nullValue());
322-
assertThat(searchHit.getSourceAsMap(), nullValue());
323321
assertThat(searchHit.getSourceRef(), nullValue());
324322
assertThat(searchHit.getSourceAsString(), nullValue());
325323
}

0 commit comments

Comments
 (0)