Skip to content

Release DocumentField instances in SearchHit.deallocate #124925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 18, 2025
Merged

Release DocumentField instances in SearchHit.deallocate #124925

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f73ae69
Release DocumentField instances in SearchHit.deallocate
original-brownbear Mar 14, 2025
e4b36ac
comment and more
original-brownbear Mar 14, 2025
984ac91
gotta copy unpooled
original-brownbear Mar 14, 2025
76ef286
Merge remote-tracking branch 'elastic/main' into release-doc-fields-asap
original-brownbear Mar 17, 2025
4eed47d
module
original-brownbear Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions server/src/main/java/org/elasticsearch/search/SearchHit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,13 +520,15 @@ public DocumentField removeDocumentField(String field) {
* @return a map of metadata fields for this hit
*/
public Map<String, DocumentField> getMetadataFields() {
assert hasReferences();
return Collections.unmodifiableMap(metaFields);
}

/**
* @return a map of non-metadata fields requested for this hit
*/
public Map<String, DocumentField> getDocumentFields() {
assert hasReferences();
return Collections.unmodifiableMap(documentFields);
}

Expand All @@ -535,6 +537,7 @@ public Map<String, DocumentField> getDocumentFields() {
* were required to be loaded. Includes both document and metadata fields.
*/
public Map<String, DocumentField> getFields() {
assert hasReferences();
if (metaFields.size() > 0 || documentFields.size() > 0) {
final Map<String, DocumentField> fields = new HashMap<>();
fields.putAll(metaFields);
Expand All @@ -556,6 +559,7 @@ public boolean hasLookupFields() {
* Resolve the lookup fields with the given results and merge them as regular fetch fields.
*/
public void resolveLookupFields(Map<LookupField, List<Object>> lookupResults) {
assert hasReferences();
if (lookupResults.isEmpty()) {
return;
}
Expand Down Expand Up @@ -585,6 +589,7 @@ public void resolveLookupFields(Map<LookupField, List<Object>> lookupResults) {
* A map of highlighted fields.
*/
public Map<String, HighlightField> getHighlightFields() {
assert hasReferences();
return highlightFields == null ? emptyMap() : highlightFields;
}

Expand Down Expand Up @@ -724,6 +729,17 @@ private void deallocate() {
r.decRef();
}
SearchHit.this.source = null;
clearIfMutable(documentFields);
clearIfMutable(metaFields);
this.highlightFields = null;
}

private static void clearIfMutable(Map<String, DocumentField> fields) {
// check that we're dealing with a HashMap, instances read from the wire that are empty be of an immutable type
assert fields instanceof HashMap<?, ?> || fields.isEmpty() : fields;
if (fields instanceof HashMap<?, ?> hm) {
Copy link
Contributor

@andreidan andreidan Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just check if the fields map is empty as opposed to an instanceof check? (note that EmptyMap.clear() doesn't throw, however a potential replacement with List.of() would)

Copy link
Contributor Author

@original-brownbear original-brownbear Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I didn't wanna mess with the specifics of empty and not, this seemed the safest for a stop-gap improvement :) We can do much better here down the line. For now this deals with statistically speaking at least 50% of the issue.

hm.clear();
}
}

@Override
Expand Down Expand Up @@ -756,12 +772,16 @@ public SearchHit asUnpooled() {
innerHits == null
? null
: innerHits.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().asUnpooled())),
documentFields,
metaFields,
cloneIfHashMap(documentFields),
Copy link
Contributor

@andreidan andreidan Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if this is obvious, but why do we need to clone in the unpooled case?

deallocate won't be called at all for unpooled hits

Copy link
Contributor

@andreidan andreidan Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's about not sharing the references with pooled instances that will now modify the state - I got there in the end :)

cloneIfHashMap(metaFields),
ALWAYS_REFERENCED
);
}

private Map<String, DocumentField> cloneIfHashMap(Map<String, DocumentField> map) {
return map instanceof HashMap<String, DocumentField> hashMap ? new HashMap<>(hashMap) : map;
}

public boolean isPooled() {
return refCounted != ALWAYS_REFERENCED;
}
Expand Down