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

Set stack size to 16M #11327

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Set stack size to 16M #11327

merged 2 commits into from
Oct 15, 2024

Conversation

AdRiley
Copy link
Member

@AdRiley AdRiley commented Oct 15, 2024

Pull Request Description

This PR attempts to fix the SQLServer tests which seem to be right on the limit of the default stack size.

Before this change

runEngineDistribution --run C:/Code/enso/test/Microsoft_Tests/src/SQLServer_Spec.enso shown.in.the.doc.example

would stack overflow indeterministically.

If I make this change -Xss512k then that test always stack overflows.

I'm not quite sure why I need to add this change here as .jvmopts looks to already attempts to set the stack size to 16M.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@AdRiley AdRiley added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 15, 2024
@AdRiley AdRiley marked this pull request as ready for review October 15, 2024 09:54
@AdRiley AdRiley merged commit 07d0015 into develop Oct 15, 2024
35 of 36 checks passed
@AdRiley AdRiley deleted the wip/adr/increase-stack-size branch October 15, 2024 15:11
@JaroslavTulach
Copy link
Member

@AdRiley you haven't increased the size of stack when launched from project-manager - e.g. when used in the IDE the stack size will still default to Java default.

@JaroslavTulach
Copy link
Member

I'm not quite sure why I need to add this change here as .jvmopts looks to already attempts to set the stack size to 16M.

Are you talking about this .jvmopts file. I guess it is only good for sbt - at least that the impression I get from the blame output. CCing @Akirathan

@JaroslavTulach
Copy link
Member

Why have you decided to set the stack value to 16M when the experiments I have seen indicate that 2M would be enough?

@AdRiley
Copy link
Member Author

AdRiley commented Oct 16, 2024

@AdRiley you haven't increased the size of stack when launched from project-manager - e.g. when used in the IDE the stack size will still default to Java default.

@radeusgd indicated distribution/manifest.template.yaml would do this. Is there another spot?

@AdRiley
Copy link
Member Author

AdRiley commented Oct 16, 2024

Why have you decided to set the stack value to 16M when the experiments I have seen indicate that 2M would be enough?

There were existing other places in our codebase where we set it to 16M so given I was making up a number I copied those for consistency. I'm happy to change this to something smaller if you think that is better?

@JaroslavTulach
Copy link
Member

Alternative suggestion to increasing stack size would be

that would lower the amount of Java stack elements by ten to twenty per each map. @AdRiley are you using map? Is there many map & co. invocations in your stacktrace.

@JaroslavTulach
Copy link
Member

There were existing other places in our codebase where we set it to 16M

.jvmopts? That's completely unrelated.

@AdRiley
Copy link
Member Author

AdRiley commented Oct 16, 2024

Alternative suggestion to increasing stack size would be

that would lower the amount of Java stack elements by ten to twenty per each map. @AdRiley are you using map? Is there many map & co. invocations in your stacktrace.

I think there was 3. One in the test framework and 2 in my code.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Oct 16, 2024

I think there was 3. One in the test framework and 2 in my code.

That's very little. Btw. can you share the stacktrace (Java as well as Enso) somewhere? Maybe we can find other places which might benefit from an optimization (Boolean.if_then_else comes to my mind first).

Akirathan added a commit that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants