Release DocumentField instances in SearchHit.deallocate #124925
Conversation
These can consume non-trivial heap if transport messages start to queue up. Lets release them asap.
original-brownbear
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
The idea looks correct to me, as we have seen cases of e.g. script fields loading _source and filling up document fields. I wish we'd add a test that verifies we do the right thing here.
I. figured the assertions are probably good enough. The possible new bug this introduces would be accessing the fields after release and that's covered by the asserts. Would be nice to have tests for the memory effects though but that's a bit of a bigger project I'm afraid. |
There was a problem hiding this comment.
Thanks Armin, this looks good in principle.
Just left a couple of minor suggestions/questions
edit: one more thing, should we have a unit/integration test where we dec ref a searchhit to 0 and assert these fields are empty/null to make sure we don't lose this optimisation in future refactorings
| : innerHits.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().asUnpooled())), | ||
| documentFields, | ||
| metaFields, | ||
| cloneIfHashMap(documentFields), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah, it's about not sharing the references with pooled instances that will now modify the state - I got there in the end :)
| 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) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
andreidan
left a comment
andreidan
left a comment
There was a problem hiding this comment.
This LGTM, thanks Armin.
As I said before, I'd like to see a test that verifies deallocate does the correct thing (so this change is resilient to refactorings)
I'll leave it up to you to decide if you'd like to add one or not.
|
Thanks Andrei! I'll leave out the tests for now because it's a lot of test for little gain, but stay tuned I have a follow-up that deals with this problem a little more fundamentally so we can stop worrying about any special cases :) |
original-brownbear
added
the
auto-backport
💚 Backport successful
|
These can consume non-trivial heap if transport messages start to queue up. Let's release them asap.
These can consume non-trivial heap if transport messages start to queue up. Let's release them asap.
4 participants
These can consume non-trivial heap if transport messages start to queue up. Lets release them asap.