Skip to content
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

Jump to bottom

Enhance real memory circuit breaker with G1 GC #58674

Merged
merged 19 commits into from Jul 13, 2020
Merged

Enhance real memory circuit breaker with G1 GC #58674

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
05ec98a
Enhance real memory circuit breaker with G1 GC
henningandersen May 27, 2020
b6b565a
Time not monotonic.
henningandersen Jul 1, 2020
c8ac1af
Add link to JDK G1 region size calculation.
henningandersen Jul 1, 2020
1d007ca
Rename to "over limit" rather than "double check"
henningandersen Jul 1, 2020
66d642d
Merge remote-tracking branch 'origin/master' into enhance_g1_circuit_…
henningandersen Jul 1, 2020
3eacf32
Keep maxHeap in field.
henningandersen Jul 7, 2020
addeede
Improved logging.
henningandersen Jul 7, 2020
110925d
Also check young GC counts.
henningandersen Jul 7, 2020
cfde2a7
Move wiring into createDoubleCheckStrategy.
henningandersen Jul 7, 2020
fea2ddc
createOverLimitStrategy naming
henningandersen Jul 7, 2020
845dc10
Use Booleans.parseBoolean
henningandersen Jul 7, 2020
3f61f93
Always call the over limit strategy
henningandersen Jul 7, 2020
55a1774
Fix test stability
henningandersen Jul 7, 2020
ed460f9
Improved code formatting
henningandersen Jul 7, 2020
7d148d0
Better field order.
henningandersen Jul 7, 2020
263f474
Few line reorderings
henningandersen Jul 7, 2020
79bd399
Merge remote-tracking branch 'origin/master' into enhance_g1_circuit_…
henningandersen Jul 8, 2020
facd321
Added lock timeout.
henningandersen Jul 8, 2020
14e574c
Merge branch 'master' into enhance_g1_circuit_breaking
elasticmachine Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
139 changes: 137 additions & 2 deletions server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java
View file
Open in desktop
Expand Up @@ -30,7 +30,9 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.monitor.jvm.JvmInfo;

import java.lang.management.ManagementFactory;
import java.lang.management.MemoryMXBean;
Expand All @@ -40,6 +42,7 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.LongSupplier;
import java.util.stream.Collectors;

import static org.elasticsearch.indices.breaker.BreakerSettings.CIRCUIT_BREAKER_LIMIT_SETTING;
Expand Down Expand Up @@ -104,7 +107,16 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService {
// Tripped count for when redistribution was attempted but wasn't successful
private final AtomicLong parentTripCount = new AtomicLong(0);

private final DoubleCheckStrategy doubleCheckStrategy;

public HierarchyCircuitBreakerService(Settings settings, List<BreakerSettings> customBreakers, ClusterSettings clusterSettings) {
this(settings, customBreakers, clusterSettings,
// hardcode interval, do not want any tuning of it outside code changes.
createDoubleCheckStrategy(JvmInfo.jvmInfo(), HierarchyCircuitBreakerService::realMemoryUsage, System::currentTimeMillis, 5000));
}

HierarchyCircuitBreakerService(Settings settings, List<BreakerSettings> customBreakers, ClusterSettings clusterSettings,
DoubleCheckStrategy doubleCheckStrategy) {
super();
HashMap<String, CircuitBreaker> childCircuitBreakers = new HashMap<>();
childCircuitBreakers.put(CircuitBreaker.FIELDDATA, validateAndCreateBreaker(
Expand Down Expand Up @@ -168,6 +180,8 @@ public HierarchyCircuitBreakerService(Settings settings, List<BreakerSettings> c
CIRCUIT_BREAKER_OVERHEAD_SETTING,
(name, updatedValues) -> updateCircuitBreakerSettings(name, updatedValues.v1(), updatedValues.v2()),
(s, t) -> {});

this.doubleCheckStrategy = doubleCheckStrategy;
}

private void updateCircuitBreakerSettings(String name, ByteSizeValue newLimit, Double newOverhead) {
Expand Down Expand Up @@ -231,7 +245,7 @@ public CircuitBreakerStats stats(String name) {
breaker.getTrippedCount());
}

private static class MemoryUsage {
static class MemoryUsage {
final long baseUsage;
final long totalUsage;
final long transientChildUsage;
Expand Down Expand Up @@ -268,6 +282,10 @@ private MemoryUsage memoryUsed(long newBytesReserved) {

//package private to allow overriding it in tests
long currentMemoryUsage() {
return realMemoryUsage();
}

static long realMemoryUsage() {
try {
return MEMORY_MX_BEAN.getHeapMemoryUsage().getUsed();
} catch (IllegalArgumentException ex) {
Expand All @@ -290,7 +308,7 @@ public long getParentLimit() {
public void checkParentLimit(long newBytesReserved, String label) throws CircuitBreakingException {
final MemoryUsage memoryUsed = memoryUsed(newBytesReserved);
long parentLimit = this.parentSettings.getLimit();
if (memoryUsed.totalUsage > parentLimit) {
if (memoryUsed.totalUsage > parentLimit && doubleCheckMemoryUsed(memoryUsed).totalUsage > parentLimit) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the method call can be replaced with overLimitStrategy.apply(memoryUsed) and the construction of the OverLimitStrategy will be responsible for handling the behavior of what to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done in 3f61f93

this.parentTripCount.incrementAndGet();
final StringBuilder message = new StringBuilder("[parent] Data too large, data for [" + label + "]" +
" would be [" + memoryUsed.totalUsage + "/" + new ByteSizeValue(memoryUsed.totalUsage) + "]" +
Expand Down Expand Up @@ -324,6 +342,14 @@ public void checkParentLimit(long newBytesReserved, String label) throws Circuit
}
}

MemoryUsage doubleCheckMemoryUsed(MemoryUsage memoryUsed) {
if (this.trackRealMemoryUsage) {
return doubleCheckStrategy.doubleCheck(memoryUsed);
} else {
return memoryUsed;
}
}

private CircuitBreaker validateAndCreateBreaker(BreakerSettings breakerSettings) {
// Validate the settings
validateSettings(new BreakerSettings[] {breakerSettings});
Expand All @@ -334,4 +360,113 @@ private CircuitBreaker validateAndCreateBreaker(BreakerSettings breakerSettings)
this,
breakerSettings.getName());
}

private static DoubleCheckStrategy createDoubleCheckStrategy(JvmInfo jvmInfo, LongSupplier currentMemoryUsageSupplier,
LongSupplier timeSupplier, long minimumInterval) {
if (jvmInfo.useG1GC().equals("true")
Copy link
Member

Choose a reason for hiding this comment

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

In line with an earlier comment, we could pass in trackRealMemoryUsage to this method and add it to this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also part of 3f61f93

// messing with GC is "dangerous" so we apply an escape hatch. Not intended to be used.
&& Boolean.parseBoolean(System.getProperty("es.real_memory_circuit_breaker.g1.double_check.enabled", "true"))) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind using Booleans.parseBoolean(System.getProperty("es.real_memory_circuit_breaker.g1.double_check.enabled"), true)? The java Boolean parsing is pretty lenient and I thought it was a forbidden api at some point :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in 845dc10

return new G1DoubleCheckStrategy(jvmInfo, currentMemoryUsageSupplier, timeSupplier, minimumInterval);
} else {
return memoryUsed -> memoryUsed;
}
}

interface DoubleCheckStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name it OverLimitStrategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++, see 1d007ca

MemoryUsage doubleCheck(MemoryUsage memoryUsed);
}

static class G1DoubleCheckStrategy implements DoubleCheckStrategy {
private final long g1RegionSize;
private final LongSupplier currentMemoryUsageSupplier;
private final LongSupplier timeSupplier;

private long lastCheckTime = Long.MIN_VALUE;
private final long minimumInterval;

private long blackHole;
private final Object lock = new Object();

G1DoubleCheckStrategy(JvmInfo jvmInfo, LongSupplier currentMemoryUsageSupplier,
LongSupplier timeSupplier, long minimumInterval) {
assert minimumInterval > 0;
this.currentMemoryUsageSupplier = currentMemoryUsageSupplier;
this.timeSupplier = timeSupplier;
this.minimumInterval = minimumInterval;
long g1RegionSize = jvmInfo.getG1RegionSize();
if (g1RegionSize <= 0) {

this.g1RegionSize = fallbackRegionSize(jvmInfo);
} else {
this.g1RegionSize = g1RegionSize;
}
}

static long fallbackRegionSize(JvmInfo jvmInfo) {
// mimick JDK calculation
Copy link
Member

Choose a reason for hiding this comment

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

can you add a link or reference to this calculation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in c8ac1af

long averageHeapSize =
(jvmInfo.getMem().getHeapMax().getBytes() + JvmInfo.jvmInfo().getMem().getHeapMax().getBytes()) / 2;
long regionSize = Long.highestOneBit(averageHeapSize / 2048);
if (regionSize < ByteSizeUnit.MB.toBytes(1)) {
regionSize = ByteSizeUnit.MB.toBytes(1);
} else if (regionSize > ByteSizeUnit.MB.toBytes(32)) {
regionSize = ByteSizeUnit.MB.toBytes(32);
}
return regionSize;
}

@Override
public MemoryUsage doubleCheck(MemoryUsage memoryUsed) {
long maxHeap = JvmInfo.jvmInfo().getMem().getHeapMax().getBytes();
Copy link
Member

Choose a reason for hiding this comment

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

Since this appears to be a consistent value, maybe we just keep it as a final long that is a class member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 3eacf32

boolean leader;
synchronized (lock) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to use lock over this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the locality of the usage, not really, it is just a personal preference since it avoids thinking about external synchronization on this. I am OK turning it into this if you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it; just curious.

leader = timeSupplier.getAsLong() >= lastCheckTime + minimumInterval;
doubleCheckingRealMemoryUsed(leader);
if (leader) {
logger.info("attempting to trigger G1GC due to high heap usage [{}]", memoryUsed.baseUsage);
long localBlackHole = 0;
// number of allocations, corresponding to (approximately) number of free regions + 1
long allocationCount = (maxHeap - memoryUsed.baseUsage) / g1RegionSize + 1;
// allocations of half-region size becomes single humongous alloc, thus taking up a full region.
int allocationSize = (int) (g1RegionSize >> 1);
long maxUsageObserved = memoryUsed.baseUsage;
for (long i = 0; i < allocationCount; ++i) {
long current = currentMemoryUsageSupplier.getAsLong();
if (current >= maxUsageObserved) {
maxUsageObserved = current;
} else {
// we observed a memory drop, so some GC must have occurred
break;
}
localBlackHole += new byte[allocationSize].hashCode();
Copy link
Member

Choose a reason for hiding this comment

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

is it possible for this to trigger an OOM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no.

In theory yes, if there is really no collectible heap left.

But if that was the case, just creating the CircuitBreakingException poses the same risk. And if we are that close, we are doomed anyway I think. The chances of us having a workload at exactly 99.95 percent heap (corresponding to approximately 2000 regions) and surviving is so small that even if it was the case, the next time round we enter the same workload it would fall over.

Notice that we only need 1 region of space free or collectible space for this to succeed.

}

blackHole += localBlackHole;
logger.trace("black hole [{}]", blackHole);
long now = timeSupplier.getAsLong();
assert now > this.lastCheckTime;
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately neither System.currentTimeMillis() nor System.nanoTime() are always monotonic so now could be less than the last checked time so I do not believe that this assert should be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed in b6b565a

this.lastCheckTime = now;
}
}

final long current = currentMemoryUsageSupplier.getAsLong();
if (current < memoryUsed.baseUsage) {
if (leader) {
logger.info("GC did bring memory usage down, before [{}], after [{}]", memoryUsed.baseUsage, current);
}
return new MemoryUsage(current, memoryUsed.totalUsage - memoryUsed.baseUsage + current,
memoryUsed.transientChildUsage, memoryUsed.permanentChildUsage);
} else {
if (leader) {
logger.info("GC did not bring memory usage down, before [{}], after [{}]", memoryUsed.baseUsage, current);
}
// prefer original measurement when reporting if heap usage was not brought down.
return memoryUsed;
}
}

void doubleCheckingRealMemoryUsed(boolean leader) {
// for tests to override.
}
}
}
16 changes: 14 additions & 2 deletions server/src/main/java/org/elasticsearch/monitor/jvm/JvmInfo.java
View file
Open in desktop
Expand Up @@ -98,6 +98,7 @@ public class JvmInfo implements ReportingService.Info {
String onOutOfMemoryError = null;
String useCompressedOops = "unknown";
String useG1GC = "unknown";
long g1RegisionSize = -1;
String useSerialGC = "unknown";
long configuredInitialHeapSize = -1;
long configuredMaxHeapSize = -1;
Expand Down Expand Up @@ -130,6 +131,8 @@ public class JvmInfo implements ReportingService.Info {
try {
Object useG1GCVmOptionObject = vmOptionMethod.invoke(hotSpotDiagnosticMXBean, "UseG1GC");
useG1GC = (String) valueMethod.invoke(useG1GCVmOptionObject);
Object regionSizeVmOptionObject = vmOptionMethod.invoke(hotSpotDiagnosticMXBean, "G1HeapRegionSize");
g1RegisionSize = Long.parseLong((String) valueMethod.invoke(regionSizeVmOptionObject));
} catch (Exception ignored) {
}

Expand Down Expand Up @@ -180,9 +183,11 @@ public class JvmInfo implements ReportingService.Info {
onOutOfMemoryError,
useCompressedOops,
useG1GC,
useSerialGC);
useSerialGC,
g1RegisionSize);
}


@SuppressForbidden(reason = "PathUtils#get")
private static boolean usingBundledJdk() {
/*
Expand Down Expand Up @@ -229,12 +234,13 @@ public static JvmInfo jvmInfo() {
private final String useCompressedOops;
private final String useG1GC;
private final String useSerialGC;
private final long g1RegionSize;

private JvmInfo(long pid, String version, String vmName, String vmVersion, String vmVendor, boolean bundledJdk, Boolean usingBundledJdk,
long startTime, long configuredInitialHeapSize, long configuredMaxHeapSize, Mem mem, String[] inputArguments,
String bootClassPath, String classPath, Map<String, String> systemProperties, String[] gcCollectors,
String[] memoryPools, String onError, String onOutOfMemoryError, String useCompressedOops, String useG1GC,
String useSerialGC) {
String useSerialGC, long g1RegionSize) {
this.pid = pid;
this.version = version;
this.vmName = vmName;
Expand All @@ -257,6 +263,7 @@ private JvmInfo(long pid, String version, String vmName, String vmVersion, Strin
this.useCompressedOops = useCompressedOops;
this.useG1GC = useG1GC;
this.useSerialGC = useSerialGC;
this.g1RegionSize = g1RegionSize;
}

public JvmInfo(StreamInput in) throws IOException {
Expand Down Expand Up @@ -291,6 +298,7 @@ public JvmInfo(StreamInput in) throws IOException {
this.onOutOfMemoryError = null;
this.useG1GC = "unknown";
this.useSerialGC = "unknown";
this.g1RegionSize = -1;
}

@Override
Expand Down Expand Up @@ -486,6 +494,10 @@ public String useSerialGC() {
return this.useSerialGC;
}

public long getG1RegionSize() {
return g1RegionSize;
}

public String[] getGcCollectors() {
return gcCollectors;
}
Expand Down