Periodically check the available memory when fetching search hits source #121920

andreidan · 2025-02-06T15:56:12Z

When fetching documents, sometimes we need to load the
entire source of search hits. Document sources can be large,
and with support for up to 10k hits per search request, this creates
a significant untracked memory load on Elasticsearch that can
potentially cause out-of-memory errors.

This PR adds memory checking for hits source in the fetch phase.
We check with the parent (the real memory) circuit breaker every
1MiB of loaded source and when fetching the last document of every
segment. This gives the real memory breaker a chance to interrupt
running operations when we're running low on memory, and prevent
potential OOMs.

The amount of local accounting to buffer is controlled by the
search.memory_accounting_buffer_size dynamic setting and defaults to
1MiB.

Fixes #89656

This adds bytes accounting for the source loading of search hits in the request circuit breaker.

...enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichShardMultiSearchAction.java

andreidan · 2025-02-07T12:07:36Z

Restarting CI due to #121927

andreidan · 2025-02-07T12:07:44Z

@elasticmachine run elasticsearch-ci/part-2

andreidan · 2025-02-10T10:13:37Z

Current failure is #122153
Restarting tests

andreidan · 2025-02-10T10:13:45Z

@elasticmachine update branch

andreidan · 2025-02-23T13:04:05Z

Thanks for the iterations here @original-brownbear

Added unit tests and updated the PR description. I think this is now ready if you'd like to have another look.

andreidan · 2025-02-23T13:10:09Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

                if (context.isCancelled()) {
                    throw new TaskCancelledException("cancelled");
                }
+                ++processedDocs;


We could potentially not check the breaker when finishing collecting from a segment and rely on other parallel fetch operations running in the system to check the real memory breaker. Not checking at the end of a segment leaves us exposed to scenarios where we fetch the source from 600 segments and we collect 900KiB for every segment (in which case no segment will check the breaker).

If we decide to carry on as it is now and check at the end of a segment this has the potentially unwanted (or perhaps it is wanted?) side effect of checking once / segment even if the source is never requested.

edit: Thinking about it some more we can have a better compromise where we check at the end of every segment only if we requested the source. I think this minimizes the impact of this feature to only what we're looking for (targeting the loading of the source)
e.g.

(requiresSource && processedDocs == docsInLeaf)

Made this change in abacdb1 but let me know what you think @original-brownbear

Hmm as a I pointed out above, any check at a higher rate than the TLAB region size is probably not a good idea.
But now that you're asking :) Why do we even bother wit the per-leaf cost and reset the counts on a per-leaf basis. If we just don't reset on a new segment this problem goes away and the behavior is generally less affected by index geometry as well which is always nice?

As discussed, moved to per fetch phase accounting as inter segment concurrency is not available in the fetch phase.
Added minimum size of 1MiB for the buffer and no upper limit (configuring a large value could enable us to disable the feature completely)

…uired

andreidan · 2025-02-24T12:06:03Z

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


    private final RefCounted refCounted;

-    // used only in tests


This is wrong :)

andreidan · 2025-02-24T12:06:32Z

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

    }

    public static SearchHit unpooled(int nestedTopDocId, String id, NestedIdentity nestedIdentity) {
+        // always referenced search hits do NOT call #deallocate


This might seem as noise but it helped me when making sense of the pool/unpool stuff so thought maybe it's useful to future spelunkers.

original-brownbear

Looks just fine thanks Andrei! Maybe have a short chat on the setting bounds and the per-segment logic but other than that I think we're good to go here :)

original-brownbear · 2025-02-24T12:41:10Z

server/src/main/java/org/elasticsearch/search/SearchService.java

+     * This buffer is used to locally track the memory accummulate during the executiong of
+     * a search request before submitting the accumulated value to the circuit breaker.
+     */
+    public static final Setting<ByteSizeValue> MEMORY_ACCOUNTING_BUFFER_SIZE = Setting.byteSizeSetting(


I think we should give this a lower bound of 1M actually and maybe and upper bound of 32M and default it to the G1GC region size. If you think about it, checking at a finer granularity than the TLAB size really makes no sense whatsoever. Likewise, going way above the granularity makes like sense as well.
We can't really default the setting to the TLAB size cleanly I think if we make it a cluster setting so I think 1M might be an ok default since that's often our region size?
But since OpenJDK doesn't allow a lower value for the region size we probably shouldn't either.

original-brownbear · 2025-02-24T12:49:43Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

                if (context.isCancelled()) {
                    throw new TaskCancelledException("cancelled");
                }
+                ++processedDocs;


Hmm as a I pointed out above, any check at a higher rate than the TLAB region size is probably not a good idea.
But now that you're asking :) Why do we even bother wit the per-leaf cost and reset the counts on a per-leaf basis. If we just don't reset on a new segment this problem goes away and the behavior is generally less affected by index geometry as well which is always nice?

…ffer

original-brownbear

LGTM :)

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

javanna

Left a couple of nits, they can also be resolved as a follow-up, not urgent.

server/src/main/java/org/elasticsearch/search/SearchService.java

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

javanna · 2025-02-24T14:46:48Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java

+    /**
+     * Return the amount of memory to buffer locally before accounting for it in the breaker.
+     */
+    public abstract long memAccountingBufferSize();


is it necessary to add these two new methods to SearchContext? I am not quite following where they are used, are they needed mostly for testing purposes or is there more to it?

I guess what I am wondering is if we could make them perhaps arguments of the following method instead, or something along those lines, just to decrease the blast radius of this change.

We had a version where the parameters were exposed as part of SearchService#executeFetchPhase but we then need to add them in the AggregationContext (for top hits) and somehow trickle them to InnerHitsPhase.
This seemed like the cleanest way to do it. Is there a better way?

andreidan added 2 commits February 6, 2025 15:55

Account for the SearchHit source in circuit breaker

8a37ca9

This adds bytes accounting for the source loading of search hits in the request circuit breaker.

Add source memory accounting for enrich source

9b524a3

andreidan added WIP :Search Foundations/Search Catch all for Search Foundations labels Feb 6, 2025

elasticsearchmachine added the v9.1.0 label Feb 6, 2025

andreidan commented Feb 6, 2025

View reviewed changes

...enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichShardMultiSearchAction.java Outdated Show resolved Hide resolved

andreidan added 7 commits February 6, 2025 16:00

spotless

64a696a

test compile

19592ef

Test compilation

40fc178

test compile

72d2c0e

no sysout

55f40ef

DecRef for newly created hit on circuit breaking exception

8b1f8c1

FetchPhase doc iterator lets CircuitBreakingException bubble up

edaeebe

andreidan added 5 commits February 7, 2025 14:02

Merge branch 'main' into source-mem-accounting

4e3d55f

DecRef after hit creation

ff6ab24

Purge hits on CBE

c103451

Enrich purge hits on CBE

d4b22e1

Merge branch 'main' into source-mem-accounting

667873d

elasticmachine and others added 8 commits February 10, 2025 11:13

Merge branch 'main' into source-mem-accounting

86dccd6

Merge branch 'main' into source-mem-accounting

fcf7711

Use a mem accountign ref counted for the parent, unfiltered, source

ffe9053

Use unfiltered parent source ref counted

4083d64

Compile

8831a33

Compile

6ffec2d

Use the existing refCounted field in SearchHit

f4e111b

Leaktracker.wrap

b64bed5

andreidan and others added 4 commits February 23, 2025 11:59

Revert use of fetch phase source loader trip

41b74cb

Add unit test for the tracking memory in the fetch phase

6e60c6e

[CI] Auto commit changes from spotless

1469c33

Unit test the search context checkRealMemoryCB method

39a0cc0

andreidan requested a review from original-brownbear February 23, 2025 13:03

andreidan commented Feb 23, 2025

View reviewed changes

andreidan added 2 commits February 23, 2025 13:21

Only check the breaker at the end of the segment if the source is req…

abacdb1

…uired

Merge branch 'main' into source-mem-accounting

acf2f63

andreidan changed the title ~~Account for the SearchHit source in circuit breaker~~ Periodically check the available memory when fetching search hits source Feb 24, 2025

andreidan requested a review from drempapis February 24, 2025 08:30

andreidan commented Feb 24, 2025

View reviewed changes

original-brownbear reviewed Feb 24, 2025

View reviewed changes

andreidan added 3 commits February 24, 2025 14:06

Account per fetch phase and add min value for the local acocunting bu…

fab8b9b

…ffer

Spotless

01f4c90

Drop the flag on finalizing local accounting

0be02e5

andreidan requested a review from original-brownbear February 24, 2025 14:21

original-brownbear approved these changes Feb 24, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java Outdated Show resolved Hide resolved

andreidan added 2 commits February 24, 2025 14:23

Drop leftover

cff3ad8

Merge branch 'main' into source-mem-accounting

47aac2a

andreidan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 24, 2025

javanna reviewed Feb 24, 2025

View reviewed changes

andreidan removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 24, 2025

Renamings

13715a5

andreidan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 24, 2025

elasticsearchmachine merged commit 760b231 into elastic:main Feb 24, 2025
17 checks passed

andreidan deleted the source-mem-accounting branch February 24, 2025 16:26

This was referenced Mar 14, 2025

Scripted fields can cause OOMs when in combination with a large size parameter #120459

Open

Search responses with large size can cause OOMs #110962

Closed

Periodically check the available memory when fetching search hits source #121920

Periodically check the available memory when fetching search hits source #121920

Uh oh!

Conversation

andreidan commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

andreidan commented Feb 7, 2025

Uh oh!

andreidan commented Feb 7, 2025

Uh oh!

andreidan commented Feb 10, 2025

Uh oh!

andreidan commented Feb 10, 2025

Uh oh!

andreidan commented Feb 23, 2025

Uh oh!

andreidan Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andreidan commented Feb 6, 2025 •

edited

Loading

andreidan Feb 23, 2025 •

edited

Loading