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

Use fibers to guarantee stack size on Windows #5873

Merged
merged 12 commits into from
Apr 2, 2021

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Apr 1, 2021

We used to use #pragma comment(linker, "/STACK:8388608") to enforce an 8MB stack on Windows. Unfortunately, clang-cl and lld-link don't support this pragma. I have opened a PR with upstream LLVM to correct this[1], but it is clear we need a more robust solution than a transitive linker flag.

This PR adds a helper, run_with_large_stack to execute a std::function<void()> in a context where it will have at least 8 MB of stack space. Currently, it merely calls the function on Linux, but on Windows it creates a new fiber with the appropriate stack size and runs it there, blocking until the fiber returns.

It is applied in two places: around lower() and around the call to compile_func in CodeGen LLVM. This was sufficient to build locally on Windows, but there might be more places it's required. We'll let the buildbots sort that out.

[1] https://reviews.llvm.org/D99680

src/Util.cpp Outdated Show resolved Hide resolved
@alexreinking
Copy link
Member Author

Looks like some of the workers were not started in a virtualenv or otherwise numpy is not installed.

@alexreinking
Copy link
Member Author

I just got paranoid about what happens if the function throws an exception. Is it valid to catch the exception, move it to the parent fiber and then re-throw it?

@abadams
Copy link
Member

abadams commented Apr 1, 2021

Looks like Raymond Chen wrote a series of blog posts on our use case. Part 4 seems to cover exceptions. https://devblogs.microsoft.com/oldnewthing/20200605-00/?p=103840

@alexreinking
Copy link
Member Author

Looks like Raymond Chen wrote a series of blog posts on our use case. Part 4 seems to cover exceptions. https://devblogs.microsoft.com/oldnewthing/20200605-00/?p=103840

Sorry I didn't see this before implementing. I'll take a look, but does this look wrong to you?

@alexreinking
Copy link
Member Author

Oh, good, looks like I did the right thing. Raymond's implementation supports return values, which is nice, but maybe overkill for what we're doing.

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

LGTM. Would sure be nice if there was a way to do something similar on Linux/OSX/etc so that we could avoid requiring the linker flags there,too.

src/Util.cpp Outdated Show resolved Hide resolved
@alexreinking
Copy link
Member Author

LGTM. Would sure be nice if there was a way to do something similar on Linux/OSX/etc so that we could avoid requiring the linker flags there,too.

We don't require any stack-size linker flags on Linux, though, do we?

@abadams
Copy link
Member

abadams commented Apr 1, 2021

LGTM. Would sure be nice if there was a way to do something similar on Linux/OSX/etc so that we could avoid requiring the linker flags there,too.

We don't require any stack-size linker flags on Linux, though, do we?

That's correct. 8MB is the default already.

We do have a custom stack size for the fft app on os x only, but that's to reduce it substantially to act as a canary if we add a new pass that uses too much stack.

@alexreinking alexreinking added release_notes For changes that may warrant a note in README for official releases. usability labels Apr 1, 2021
@alexreinking alexreinking added this to the v12.0.0 milestone Apr 1, 2021
@alexreinking
Copy link
Member Author

@steven-johnson looks like we've got some zombies?

@steven-johnson
Copy link
Contributor

@steven-johnson looks like we've got some zombies?

It's zombies all the way down.

I've commented on an existing bug on buildbot re: the zombies that won't die.
I've open a ticket at Google re: the network that is dropping intermittently.

Unfortunately, the only way to kill the zombies is to stop and restart the master, which I'll do now.

@alexreinking
Copy link
Member Author

Thanks! Looks like all the checks are passing now... good to merge?

@steven-johnson
Copy link
Contributor

LGTM

@alexreinking alexreinking merged commit 59a04e4 into master Apr 2, 2021
@alexreinking alexreinking deleted the windows/fiber-lowering branch May 5, 2021 05:23
@alexreinking alexreinking removed the release_notes For changes that may warrant a note in README for official releases. label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants