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

Reduce memory usage of Thread.interrupted() tests #404

Merged
merged 2 commits into from
May 7, 2021

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented May 7, 2021

For some reason I still don't fully understand, the tests added in PR #398 to the started consuming more than 4gb of RAM after being cherry-picked to the v0.1.6 branch, which would cause the build fail since the JVM was limited to 4gb. I tried increasing the RAM allocated to the JVM but wasn't able to get it to run correctly until 6gb and 7gb is the maximum amount of memory allowed by GitHub actions, which I know we will likely migrate to some day soon. That was cutting it pretty close, so I was felt it was better to reduce memory consumption of the tests in that branch and decided to apply a similar fix to master in this PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

These test also execute significantly faster.
@dlurton dlurton requested a review from alancai98 May 7, 2021 22:13
@dlurton dlurton self-assigned this May 7, 2021
@dlurton dlurton requested a review from abhikuhikar May 7, 2021 22:19
@dlurton dlurton merged commit 53b8587 into master May 7, 2021
Copy link
Contributor

@almann almann left a comment

Choose a reason for hiding this comment

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

LGTM, minor questions below.

Comment on lines +48 to +50
private val reallyBigNAry = makeBigExprNode(20000000)
private val bigNAry = makeBigExprNode(10000000)
private val bigPartiqlAst = makeBigPartiqlAstExpr(10000000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the laziness the problem or just a cleanup change?

}


class FakeList<T>(override val size: Int, private val item: T) : AbstractList<T>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment would be helpful but this Fake list is just logically a single item expanded N times, right?

@dlurton dlurton deleted the thread-interrupted-test-memory branch May 10, 2021 19:55
@dlurton dlurton added this to the v0.3.0 milestone Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants