Skip to content

Conversation

@danielmitterdorfer
Copy link
Member

@danielmitterdorfer danielmitterdorfer commented Oct 5, 2018

With this commit we restore the previous behavior in
BigArraysTests#testMaxSizeExceededOnResize but lower the sizes that
are tested to the range between 256 bytes to 16 kB so the test does not
produce a whole lot of garbage.

The previous attempt to reduce the amount of garbage produced by that
test was to properly size the array initially but it failed to account
for object alignment which lead to test failures in some cases. While it
would be possible to account for object alignment, we would need to open
up BigArrays or directly use the underlying Lucene API which would
require us to allocate an array upfront only to determine its size (incl.
object alignment).

Instead we have fixed this issue by conservatively sizing the array
initially (so the initial allocation will never trip the circuit
breaker) and reduce garbage by reducing the circuit breaker's upper
bound as described previously.

Closes #33750

With this commit we restore the previous behavior in
`BigArraysTests#testMaxSizeExceededOnResize` but lower the sizes that
are tested to the range between 256 bytes to 16 kB so the test does not
produce a whole lot of garbage.

The previous attempt to reduce the amount of garbage produced by that
test was to properly size the array initially but it failed to account
for object alignment which lead to test failures in some cases. While it
would be possible to account for object alignment, we would need to open
up BigArrays or directly use the underlying Lucene API which would
require us to allocate an array upfront only to find its size (incl.
object alignment).

Instead we have fixed this issue by conservatively sizing the array
initially (so the initial allocation will never trip the circuit
breaker) and reduce garbage by reducing the circuit breaker's upper
bound as described previously.

Closes elastic#33750
@danielmitterdorfer danielmitterdorfer added >test Issues or PRs that are addressing/adding tests review :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0 labels Oct 5, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@danielmitterdorfer danielmitterdorfer merged commit 7d82691 into elastic:master Oct 5, 2018
@danielmitterdorfer danielmitterdorfer deleted the bigarrays-cb branch October 5, 2018 13:39
danielmitterdorfer added a commit that referenced this pull request Oct 5, 2018
With this commit we restore the previous behavior in
`BigArraysTests#testMaxSizeExceededOnResize` but lower the sizes that
are tested to the range between 256 bytes to 16 kB so the test does not
produce a whole lot of garbage.

The previous attempt to reduce the amount of garbage produced by that
test was to properly size the array initially but it failed to account
for object alignment which lead to test failures in some cases. While it
would be possible to account for object alignment, we would need to open
up BigArrays or directly use the underlying Lucene API which would
require us to allocate an array upfront only to find its size (incl.
object alignment).

Instead we have fixed this issue by conservatively sizing the array
initially (so the initial allocation will never trip the circuit
breaker) and reduce garbage by reducing the circuit breaker's upper
bound as described previously.

Closes #33750
Relates #34325
@danielmitterdorfer
Copy link
Member Author

Backported to 6.x in d5f202f.

kcm pushed a commit that referenced this pull request Oct 30, 2018
With this commit we restore the previous behavior in
`BigArraysTests#testMaxSizeExceededOnResize` but lower the sizes that
are tested to the range between 256 bytes to 16 kB so the test does not
produce a whole lot of garbage.

The previous attempt to reduce the amount of garbage produced by that
test was to properly size the array initially but it failed to account
for object alignment which lead to test failures in some cases. While it
would be possible to account for object alignment, we would need to open
up BigArrays or directly use the underlying Lucene API which would
require us to allocate an array upfront only to find its size (incl.
object alignment).

Instead we have fixed this issue by conservatively sizing the array
initially (so the initial allocation will never trip the circuit
breaker) and reduce garbage by reducing the circuit breaker's upper
bound as described previously.

Closes #33750
Relates #34325
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload >test Issues or PRs that are addressing/adding tests v6.5.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

testMaxSizeExceededOnResize fails to create array to resize

4 participants