-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ESQL: Limit memory usage of fold
#118602
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.
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
ESQL: Limit memory usage of fold
#118602
Changes from 19 commits
8fa64b1
8ccd678
a126e84
bc661ce
28ac84f
facd394
17d2fc4
faf0e5e
dd9eeba
f46447e
739e9bf
6660b88
b9ea47f
2e24625
5a7fe2e
2d063c8
115477a
bfbc7eb
f3fd791
fa12629
d790556
2abbeef
39440d8
0d5afb1
b20f541
456833d
573a238
5742c23
b43b8d8
0651b78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 118602 | ||
summary: Limit memory usage of `fold` | ||
area: ES|QL | ||
type: bug | ||
issues: [] |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.esql.core.expression; | ||
|
||
import org.elasticsearch.common.breaker.CircuitBreaker; | ||
import org.elasticsearch.common.breaker.CircuitBreakingException; | ||
import org.elasticsearch.common.unit.ByteSizeValue; | ||
import org.elasticsearch.core.Releasable; | ||
import org.elasticsearch.xpack.esql.core.QlClientException; | ||
import org.elasticsearch.xpack.esql.core.tree.Source; | ||
|
||
/** | ||
* Context passed to {@link Expression#fold}. This is not thread safe. | ||
*/ | ||
public class FoldContext { | ||
/** | ||
* {@link Expression#fold} using any amount of memory. Only safe for tests. | ||
*/ | ||
public static FoldContext unbounded() { | ||
return new FoldContext(Long.MAX_VALUE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are still using it in a few places, wouldn't it make sense to default it to the same 5% of the memory as the default in the query pragmas (or a slightly higher value), rather than making it really unbounded? It's a bit paranoid maybe, I guess the final goal here is safety There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hopeful we can remove the usages in a follow-up. Let's get this in an have a conversation about whether or not we should replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we want to give this the 5% limit before merging this? |
||
} | ||
|
||
private final long initialAllowedBytes; | ||
private long allowedBytes; | ||
alex-spies marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public FoldContext(long allowedBytes) { | ||
this.initialAllowedBytes = allowedBytes; | ||
this.allowedBytes = allowedBytes; | ||
} | ||
|
||
/** | ||
* Track an allocation. Best to call this <strong>before</strong> allocating | ||
* if possible, but after is ok if the allocation is small. | ||
* <p> | ||
* Note that, unlike {@link CircuitBreaker}, you don't <strong>have</strong> | ||
* to free this allocation later. This is important because the query plan | ||
* doesn't implement {@link Releasable} so it <strong>can't</strong> free | ||
* consistently. But when you have to allocate big chunks of memory during | ||
* folding and know that you are returning the memory it is kindest to | ||
* call this with a negative number, effectively giving those bytes back. | ||
* </p> | ||
*/ | ||
public void trackAllocation(Source source, long bytes) { | ||
allowedBytes -= bytes; | ||
assert allowedBytes <= initialAllowedBytes : "returned more bytes than it used"; | ||
if (allowedBytes < 0) { | ||
throw new FoldTooMuchMemoryException(source, bytes, initialAllowedBytes); | ||
} | ||
} | ||
|
||
/** | ||
* Adapt this into a {@link CircuitBreaker} suitable for building bounded local | ||
* DriverContext. This is absolutely an abuse of the {@link CircuitBreaker} contract | ||
* and only methods used by BlockFactory are implemented. And this'll throw a | ||
* {@link FoldTooMuchMemoryException} instead of the standard {@link CircuitBreakingException}. | ||
* This works for the common folding implementation though. | ||
*/ | ||
public CircuitBreaker circuitBreakerView(Source source) { | ||
return new CircuitBreaker() { | ||
@Override | ||
public void circuitBreak(String fieldName, long bytesNeeded) { | ||
throw new UnsupportedOperationException(); | ||
} | ||
|
||
@Override | ||
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException { | ||
trackAllocation(source, bytes); | ||
alex-spies marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Override | ||
public void addWithoutBreaking(long bytes) { | ||
assert bytes <= 0 : "we only expect this to be used for deallocation"; | ||
allowedBytes -= bytes; | ||
assert allowedBytes <= initialAllowedBytes : "returned more bytes than it used"; | ||
} | ||
|
||
@Override | ||
public long getUsed() { | ||
/* | ||
* This isn't expected to be used by we can implement it so we may as | ||
* well. Maybe it'll be useful for debugging one day. | ||
*/ | ||
return initialAllowedBytes - allowedBytes; | ||
} | ||
|
||
@Override | ||
public long getLimit() { | ||
/* | ||
* This isn't expected to be used by we can implement it so we may as | ||
* well. Maybe it'll be useful for debugging one day. | ||
*/ | ||
return initialAllowedBytes; | ||
} | ||
|
||
@Override | ||
public double getOverhead() { | ||
return 1.0; | ||
} | ||
|
||
@Override | ||
public long getTrippedCount() { | ||
return 0; | ||
} | ||
|
||
@Override | ||
public String getName() { | ||
return REQUEST; | ||
} | ||
|
||
@Override | ||
public Durability getDurability() { | ||
throw new UnsupportedOperationException(); | ||
} | ||
|
||
@Override | ||
public void setLimitAndOverhead(long limit, double overhead) { | ||
throw new UnsupportedOperationException(); | ||
} | ||
}; | ||
} | ||
|
||
public static class FoldTooMuchMemoryException extends QlClientException { | ||
protected FoldTooMuchMemoryException(Source source, long bytesForExpression, long initialAllowedBytes) { | ||
super( | ||
"line {}:{}: Folding query used more than {}. The expression that pushed past the limit is [{}] which needed {}.", | ||
source.source().getLineNumber(), | ||
source.source().getColumnNumber(), | ||
ByteSizeValue.ofBytes(initialAllowedBytes), | ||
source.text(), | ||
ByteSizeValue.ofBytes(bytesForExpression) | ||
); | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.