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.

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

Checks the circuit breaker before allocating bytes for a new big array #25010

Merged
merged 4 commits into from Jun 2, 2017

Conversation

Projects
None yet
5 participants
@abeyad
Copy link
Contributor

commented Jun 1, 2017

Previously, when allocating bytes for a BigArray, the array was created
(or attempted to be created) and only then would the array be checked
for the amount of RAM used to see if the circuit breaker should trip.

This is problematic because for very large arrays, if creating or
resizing the array, it is possible to attempt to create/resize and get
an OOM error before the circuit breaker trips, because the allocation
happens before checking with the circuit breaker.

This commit ensures that the circuit breaker is checked before all big
array allocations (note, this does not effect the array allocations that
are less than 16kb which use the [Type]ArrayWrapper classes found in
BigArrays.java). If such an allocation or resizing would cause the
circuit breaker to trip, then the breaker trips before attempting to
allocate and potentially running into an OOM error from the JVM.

Closes #24790

Ali Beyad
Checks the circuit breaker before allocating bytes for a new big array
Previously, when allocating bytes for a BigArray, the array was created
(or attempted to be created) and only then would the array be checked
for the amount of RAM used to see if the circuit breaker should trip.

This is problematic because for very large arrays, if creating or
resizing the array, it is possible to attempt to create/resize and get
an OOM error before the circuit breaker trips, because the allocation
happens before checking with the circuit breaker.

This commit ensures that the circuit breaker is checked before all big
array allocations (note, this does not effect the array allocations that
are less than 16kb which use the [Type]ArrayWrapper classes found in
BigArrays.java).  If such an allocation or resizing would cause the
circuit breaker to trip, then the breaker trips before attempting to
allocate and potentially running into an OOM error from the JVM.

Closes #24790

@abeyad abeyad force-pushed the abeyad:circuit_breaker_big_arrays branch to 6a690f0 Jun 1, 2017

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

@dakrone I rebased on master to get the latest changes and pushed it up

@dakrone
Copy link
Member

left a comment

I left some comments, thanks for doing this Ali!

I wish there were an easy way to do it without having to make the BigArrays @Nullable just so we can have ESTIMATOR instances, but I couldn't think of an easy way to do it :(

.build(),
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS));
BigArrays bigArrays = new BigArrays(null, hcbs, false).withCircuitBreaking();
Method create = BigArrays.class.getMethod("new" + type + "Array", long.class);

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 1, 2017

Member

Reflection? :(

What about instead of using an array of Byte, Int, etc which seems really fragile, we use a list of lambdas that invoke the real method. That way if there is ever a refactoring it will fail during compilation rather than at runtime.

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

Good point - I used reflection as that is what the two tests above it used. I will change all the tests to use lamdas instead.

.build(),
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS));
BigArrays bigArrays = new BigArrays(null, hcbs, false);
Method create = BigArrays.class.getMethod("new" + type + "Array", long.class);

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 1, 2017

Member

Same here about using lambdas instead of reflection

create.invoke(bigArrays, size);
fail("circuit breaker should trip");
} catch (InvocationTargetException e) {
assertTrue(e.getCause() instanceof CircuitBreakingException);

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 1, 2017

Member

Can you assert the getBytesWanted() and getByteLimit() are also correct here?

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

Added

* without tripping
* without tripping.
*/
void adjustBreaker(final long delta) {

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 1, 2017

Member

How would you feel about removing this and making explicit the dataAlreadyCreated parameter?

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

I prefer that, I made the change

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

I wish there were an easy way to do it without having to make the BigArrays @nullable just so we can have ESTIMATOR instances, but I couldn't think of an easy way to do it :(

I agree, I struggled with this one. I tried other approaches including making the pageShift and pageMask static fields in the individual implementations of AbstractBigArray but none of them turned out any cleaner. If you think of anything else that you'd prefer, I'm happy to try it.

Ali Beyad
@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

@dakrone I pushed 9dcf6f5 to address your feedback.

@jpountz
Copy link
Contributor

left a comment

It looks good overall even though I agree with @dakrone it is a pity that we have to make the BigArrays parameter nullable.

array.resize(newSize);
adjustBreaker(array.ramBytesUsed() - oldMemSize);

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 2, 2017

Contributor

could we add an assert that array.ramBytesUsed() is equal to the estimate we computed previously?

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

Added

private final BigArrays bigArrays;
public final boolean clearOnResize;
private final AtomicBoolean closed = new AtomicBoolean(false);

AbstractArray(BigArrays bigArrays, boolean clearOnResize) {
AbstractArray(@Nullable BigArrays bigArrays, boolean clearOnResize) {

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 2, 2017

Contributor

I agree with @dakrone that we need to find another way

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 2, 2017

Contributor

Would it work if we passed BigArrays.NON_RECYCLING_INSTANCE instead for the estimators?

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

@jpountz I completely missed the NON_RECYCLING_INSTANCE! It works perfectly. I made the change.

Ali Beyad
@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

@dakrone

dakrone approved these changes Jun 2, 2017

Copy link
Member

left a comment

Thanks @abeyad, I left a couple more comments but this looks good to me!

@@ -435,15 +440,18 @@ public CircuitBreakerService breakerService() {

private <T extends AbstractBigArray> T resizeInPlace(T array, long newSize) {
final long oldMemSize = array.ramBytesUsed();
assert oldMemSize == array.ramBytesEstimated(array.size) :
"ram bytes used should equal that which was previously estimated";

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Member

Can you add the oldMemSize and new estimate to the assertion message? (In case it fails)

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

added

@@ -435,15 +440,18 @@ public CircuitBreakerService breakerService() {

private <T extends AbstractBigArray> T resizeInPlace(T array, long newSize) {
final long oldMemSize = array.ramBytesUsed();
assert oldMemSize == array.ramBytesEstimated(array.size) :
"ram bytes used should equal that which was previously estimated";
final long estimatedIncreaseInBytes = array.ramBytesEstimated(newSize) - oldMemSize;

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Member

Can we add an assertion that this is never negative? I don't think it ever will be, but just to be sure...

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

added

@@ -91,7 +90,7 @@ public void close() {

private abstract static class AbstractArrayWrapper extends AbstractArray implements BigArray {

protected static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(ByteArrayWrapper.class);
static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(ByteArrayWrapper.class);

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Member

I don't think anything uses this SHALLOW_SIZE, so I think it can be private?

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Author Contributor

@dakrone it needs to be package-private because the other array wrapper classes (e.g. ByteArrayWrapper access it. Making it private throws compilation errors.

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Member

Ahh okay, I missed that usage then, thanks!

Ali Beyad
@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

@dakrone I pushed 3579505

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

thanks for the reviews @dakrone @jpountz!

@dakrone

dakrone approved these changes Jun 2, 2017

Copy link
Member

left a comment

Still LGTM :)

@abeyad abeyad merged commit e024c67 into elastic:master Jun 2, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@abeyad abeyad deleted the abeyad:circuit_breaker_big_arrays branch Jun 2, 2017

abeyad pushed a commit that referenced this pull request Jun 2, 2017

Ali Beyad
Checks the circuit breaker before allocating bytes for a new big array (
#25010)

Previously, when allocating bytes for a BigArray, the array was created
(or attempted to be created) and only then would the array be checked
for the amount of RAM used to see if the circuit breaker should trip.

This is problematic because for very large arrays, if creating or
resizing the array, it is possible to attempt to create/resize and get
an OOM error before the circuit breaker trips, because the allocation
happens before checking with the circuit breaker.

This commit ensures that the circuit breaker is checked before all big
array allocations (note, this does not effect the array allocations that
are less than 16kb which use the [Type]ArrayWrapper classes found in
BigArrays.java).  If such an allocation or resizing would cause the
circuit breaker to trip, then the breaker trips before attempting to
allocate and potentially running into an OOM error from the JVM.

Closes #24790

abeyad pushed a commit that referenced this pull request Jun 2, 2017

Ali Beyad
Checks the circuit breaker before allocating bytes for a new big array (
#25010)

Previously, when allocating bytes for a BigArray, the array was created
(or attempted to be created) and only then would the array be checked
for the amount of RAM used to see if the circuit breaker should trip.

This is problematic because for very large arrays, if creating or
resizing the array, it is possible to attempt to create/resize and get
an OOM error before the circuit breaker trips, because the allocation
happens before checking with the circuit breaker.

This commit ensures that the circuit breaker is checked before all big
array allocations (note, this does not effect the array allocations that
are less than 16kb which use the [Type]ArrayWrapper classes found in
BigArrays.java).  If such an allocation or resizing would cause the
circuit breaker to trip, then the breaker trips before attempting to
allocate and potentially running into an OOM error from the JVM.

Closes #24790
@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

5.x commit: 59b09c2
5.4 commit: c67c5c7

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

A bit late to the party, but 👍 :)

@clintongormley clintongormley added v6.0.0-alpha2 and removed v6.0.0 labels Jun 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.