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

Made Stack Size parametrable (for #3760) #3786

Merged
merged 4 commits into from
Apr 21, 2023
Merged

Made Stack Size parametrable (for #3760) #3786

merged 4 commits into from
Apr 21, 2023

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Apr 19, 2023

Made Stack Size parametrable.

By default, Stacks are 1MB. It is generaly a good default value, but it might be to small for some specific case, where recursion is (ab)used a lot, like for ticket #3760 .
In some other special case, with a lot of thread, smaller value might also be prefered.

Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

I think it should be fine to make this a global variable here instead of trying to thread the stack size through Tumables.

We should probably use an atomic or Mutex for the variable so we can drop the unsafe, though.

lib/vm/src/trap/traphandlers.rs Outdated Show resolved Hide resolved
@theduke
Copy link
Contributor

theduke commented Apr 19, 2023

I disagree, we very well may want different stack sizes per instance.

I can think of a concrete use case where users can configure the stack size per workload...

A global atomic won't work when multiple threads are constructing instances.

In general I'm not a fan of a global mutable setting.

This should be threaded through, even though it's annoying.

Note that tunables isn't ideal here either, because that would require constructing a separate engine.

@ptitSeb , does the chosen stack size affect compilation?

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Apr 19, 2023

I really really really want to stress out that the function call in question is very low level, so accessing high level structure like tunable or whatever other function could be a challenge.

And also, changing Stacksize on the fly should not be attempted. We are talking about stack here.
Maybe I should add some test only allow 1 change of stack size per run?

@theduke no, the stack size doesn't not change the compilation emited.

@theduke
Copy link
Contributor

theduke commented Apr 19, 2023

I'm not talking about changing the stack size for an existing instance, just configuring the size for a newly created one.

@ptitSeb ptitSeb requested a review from Michael-F-Bryan April 21, 2023 10:16
@ptitSeb ptitSeb enabled auto-merge (squash) April 21, 2023 11:23
@ptitSeb ptitSeb merged commit b2446c1 into master Apr 21, 2023
@ptitSeb ptitSeb deleted the feat_stack_size branch April 21, 2023 11:28
@ptitSeb ptitSeb added this to the v3.2.1 milestone Apr 21, 2023
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