Harden painless test against "fun" caching #24077
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The JVM caches `Integer` objects. This is known. A test in Painless was relying on the JVM not caching the particular integer `1000`. It turns out that when you provide `-XX:+AggressiveOpts` the JVM *does* cache `1000`, causing the test to fail when that is specified. This replaces `1000` with a randomly selected integer that we test to make sure *isn't* cached by the JVM. *Hopefully* this test is good enough. It relies on the caching not changing in between when we check that the value isn't cached and when we run the painless code. The cache now is a simple array but there is nothing preventing it from changing. If it does change in a way that thwarts this test then the test fail fail again. At least when that happens the next person can see the comment about how it is important that the integer isn't cached and can follow that line of inquiry. Closes elastic#24041
Member
Author
|
Thanks to user2237683 over at http://stackoverflow.com/questions/15052216/how-large-is-the-integer-cache/15773195#15773195 for digging into |
nik9000
added
review
v5.4.0
v6.0.0-alpha1
>test
Member
Author
|
I believe this also needs to go to the 5.3 branch but I'm not sure about timing. |
jdconrad
approved these changes
Apr 12, 2017
Contributor
jdconrad
left a comment
jdconrad
left a comment
There was a problem hiding this comment.
LGTM. Thanks for hunting this down!
jasontedor
approved these changes
Apr 13, 2017
Member
jasontedor
left a comment
jasontedor
left a comment
There was a problem hiding this comment.
Cool, great find @nik9000. LGTM.
Member
Author
|
Thanks for reviewing @jdconrad and @jasontedor! |
nik9000
added a commit
that referenced
this pull request
Apr 17, 2017
The JVM caches `Integer` objects. This is known. A test in Painless was relying on the JVM not caching the particular integer `1000`. It turns out that when you provide `-XX:+AggressiveOpts` the JVM *does* cache `1000`, causing the test to fail when that is specified. This replaces `1000` with a randomly selected integer that we test to make sure *isn't* cached by the JVM. *Hopefully* this test is good enough. It relies on the caching not changing in between when we check that the value isn't cached and when we run the painless code. The cache now is a simple array but there is nothing preventing it from changing. If it does change in a way that thwarts this test then the test fail fail again. At least when that happens the next person can see the comment about how it is important that the integer isn't cached and can follow that line of inquiry. Closes #24041
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Apr 18, 2017
We had a TODO about adding tests around cached boxing. In elastic#24077 I tracked down the uncached boxing tests and saw the TODO. Cached boxing testing is a fairly small extension to that work.
nik9000
added a commit
that referenced
this pull request
Apr 21, 2017
The JVM caches `Integer` objects. This is known. A test in Painless was relying on the JVM not caching the particular integer `1000`. It turns out that when you provide `-XX:+AggressiveOpts` the JVM *does* cache `1000`, causing the test to fail when that is specified. This replaces `1000` with a randomly selected integer that we test to make sure *isn't* cached by the JVM. *Hopefully* this test is good enough. It relies on the caching not changing in between when we check that the value isn't cached and when we run the painless code. The cache now is a simple array but there is nothing preventing it from changing. If it does change in a way that thwarts this test then the test fail fail again. At least when that happens the next person can see the comment about how it is important that the integer isn't cached and can follow that line of inquiry. Closes #24041
nik9000
added a commit
that referenced
this pull request
Oct 10, 2017
We had a TODO about adding tests around cached boxing. In #24077 I tracked down the uncached boxing tests and saw the TODO. Cached boxing testing is a fairly small extension to that work.
nik9000
added a commit
that referenced
this pull request
Oct 10, 2017
We had a TODO about adding tests around cached boxing. In #24077 I tracked down the uncached boxing tests and saw the TODO. Cached boxing testing is a fairly small extension to that work.
clintongormley
added
:Core/Infra/Scripting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The JVM caches
Integerobjects. This is known. A test in Painlesswas relying on the JVM not caching the particular integer
1000.In older versions of java only -127, 128 were cached. That isn't
always true anymore. It turns out that when you provide
-XX:+AggressiveOptsthe JVM does cache1000, causing thetest to fail when that is specified.
This replaces
1000with a randomly selected integer that we testto make sure isn't cached by the JVM. Hopefully this test is
good enough. It relies on the caching not changing in between when
we check that the value isn't cached and when we run the painless
code. The cache now is a simple array but there is nothing
preventing it from changing. If it does change in a way that thwarts
this test then the test fail fail again. At least when that happens
the next person can see the comment about how it is important
that the integer isn't cached and can follow that line of inquiry.
Closes #24041