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

macros: stackless expansion #36214

Merged
merged 19 commits into from
Sep 8, 2016
Merged

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Sep 2, 2016

After this PR, macro expansion cannot overflow the stack unless the expanded crate is too deep to fold.
Everything but the stackless placeholder expansion commit is also groundwork for macro modularization.

cc #35896
r? @nrc or @eddyb

@jseyfried jseyfried force-pushed the stackless_expansion branch 2 times, most recently from 6497f39 to d0695b0 Compare September 2, 2016 13:38
@durka
Copy link
Contributor

durka commented Sep 3, 2016

Hmm, this doesn't seem to be working. I tried it on my most-recursing macro, brainmunch. Having built master and this branch:

[root@li1424-173 brainmunch]# time ../rust/stackless/build/x86_64-unknown-linux-gnu/stage1/bin/rustc -O -Z time-passes src/main.rs | grep expansion
time: 8.082; rss: 68MB  expansion

real    0m9.118s
user    0m8.993s
sys     0m0.100s
[root@li1424-173 brainmunch]# time ../rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc -o master -O -Z time-passes src/main.rs | grep expansion
time: 8.501; rss: 67MB  expansion

real    0m9.575s
user    0m9.450s
sys     0m0.103s

This branch is first, then master. I would have expected a more impressive time difference and a smaller memory footprint if the expansion is really stackless. Furthermore, I removed #![recursion_limit="5000"] from main.rs and both builds immediately bailed having reached the recursion limit.

@eddyb
Copy link
Member

eddyb commented Sep 3, 2016

@durka If you don't get a stack overflow with a high recursion limit before this PR, then there's not much to see.
You couldn't notice a large memory or time difference, all that's happening AFAICT is that very deep macro recursion would not cause stack overflows anymore because the real stack isn't used.

@jseyfried
Copy link
Contributor Author

jseyfried commented Sep 3, 2016

@durka This PR doesn't change recursion_limit checking -- if macro expansion is more recursive than the recursion limit, it is still an error.

It would probably be a good idea to raise the default recursion limit or even remove it altogether, but that's for a separate PR. Note that if we remove the recursion limit altogether, I think we should limit the total memory usage or total number of expansions so that if there really is unbounded recursion, the user will still get a nice error message (not a hang or an out of memory error). I'd be happy to help if you want to do this.

To test this branch, you can set the recursion limit to something that can't be reached in a reasonable amount of time and space, like 2^32.

This PR doesn't improve performance (I actually measured ~5% regression, but that will be made up by a later PR) nor does it improve memory usage -- after this PR, the heap will have all the information that used to be on the stack and more.

@durka
Copy link
Contributor

durka commented Sep 3, 2016

Ah I see, I got too excited.

@durka
Copy link
Contributor

durka commented Sep 3, 2016

Further investigation: I lengthened the test case to stack overflow on master after 1m27s, and it finished compiling on this branch after 5 minutes. Intentionally lowering the recursion limit produces comparable time-to-error for both branches.

@bors
Copy link
Contributor

bors commented Sep 3, 2016

☔ The latest upstream changes (presumably #35957) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the stackless_expansion branch 3 times, most recently from a9fa986 to 99c896f Compare September 5, 2016 01:23
syntax_pos::DUMMY_SP => PathBuf::new(),
_ => PathBuf::from(fld.cx.parse_sess.codemap().span_to_filename(inner)),
};
fld.cx.directory.pop();
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right - in the dummy span case you're popping an empty PathBuf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@bors
Copy link
Contributor

bors commented Sep 5, 2016

☔ The latest upstream changes (presumably #36240) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member

nrc commented Sep 6, 2016

r=me with that boilerplate factored away

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Sep 6, 2016

📌 Commit 400a4f2 has been approved by nrc

@bors
Copy link
Contributor

bors commented Sep 6, 2016

⌛ Testing commit 400a4f2 with merge d75c64d...

bors added a commit that referenced this pull request Sep 6, 2016
macros: stackless expansion

After this PR, macro expansion cannot overflow the stack unless the expanded crate is too deep to fold.
Everything but the stackless placeholder expansion commit is also groundwork for macro modularization.

r? @nrc or @eddyb
@bors
Copy link
Contributor

bors commented Sep 6, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Sep 6, 2016

📌 Commit dc01d96 has been approved by nrc

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Sep 7, 2016

📌 Commit 9ac91fa has been approved by nrc

@bors
Copy link
Contributor

bors commented Sep 8, 2016

⌛ Testing commit 9ac91fa with merge 4a26286...

bors added a commit that referenced this pull request Sep 8, 2016
macros: stackless expansion

After this PR, macro expansion cannot overflow the stack unless the expanded crate is too deep to fold.
Everything but the stackless placeholder expansion commit is also groundwork for macro modularization.

r? @nrc or @eddyb
@bors bors merged commit 9ac91fa into rust-lang:master Sep 8, 2016
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 8, 2016
@jseyfried jseyfried deleted the stackless_expansion branch September 8, 2016 20:27
@jseyfried
Copy link
Contributor Author

@DanielJCampbell This refactoring might help with expansion visualization / stepwise expansion -- feel free to ping me on IRC if you'd like to discuss.

@DanielJCampbell
Copy link
Contributor

Yeah, I will do that - I have a working solution but it's based on Rust from 6 weeks ago, looks like plenty has changed since then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants