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

Return unwind #4342

Closed
wants to merge 4 commits into from
Closed

Return unwind #4342

wants to merge 4 commits into from

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Jan 4, 2013

This is still a little preliminary but it appears to work. Resulting binaries are very similar in size .. some functions are a bit bigger (no cleanup coalescing yet) but it skips an exception-table section so total binaries are typically smaller.

r? @brson just looking for early feedback at this point. Will clean up some more and rebase before landing.

@brson
Copy link
Contributor

brson commented Jan 4, 2013

r+

@brson
Copy link
Contributor

brson commented Jan 4, 2013

I didn't expect to have to put the return unwind flag into the crate map. It looks like there are two uses: 1) deciding which mode to use to initiate failure and 2) deciding how to catch the failure in the task start wrapper.

For the first case the crate map shouldn't be needed since the code to begin failure must call a distinct runtime function anyway. The only exception to this is failure initiated directly by the runtime, which only happens on out-of-stack. The out-of-stack failure is problematic anyway and I think should instead be an abort() (#3695, #3555). The second case seems like it could be eliminated by assuming that Rust functions always return bool.

@brson
Copy link
Contributor

brson commented Jan 4, 2013

I think we are planning to maintain both unwinding strategies. Do you have an idea for how we can ensure that we don't combine two crates with different strategies?

@pcwalton
Copy link
Contributor

pcwalton commented Jan 6, 2013

We may well have to compile core and std twice, once for each unwinding strategy. We should make sure this does not negatively impact the cycle time. Perhaps the best thing to do is to compile DWARF-unwinding-based libcore and libstd only at the end of stage2, only on platforms where that's supported (i.e. not Windows), and not when make check or make stage2-rustc are requested.

@graydon
Copy link
Contributor Author

graydon commented Jan 8, 2013

@brson Interesting. I can try to rework this around the assumption that all rust functions always return bool and we just ... only check it (and propagate it) when we're compiling in return-unwind mode.

I'm surprised by the idea that we'd be maintaining both strategies. Given that it seems to work better (smaller binaries, faster compiles, works on windows, will be more friendly to binding to other languages / embedding / etc.) why would we want to keep the dw2 stuff?

(I put this behind a flag to help with transition, not on the assumption that we'd be keeping both.)

@nikomatsakis
Copy link
Contributor

I am in favor of having only one strategy. Even though long term I'd
prefer to use the return value for its intended purpose, if using it to
indicate failure works better (and it seems that it does, at least for
now) let's just maintain that.

@brson
Copy link
Contributor

brson commented Jan 14, 2013

@graydon I'm not sure where I got the idea that we would maintain both, though I continue to assume that DWARF unwinding has less runtime performance impact. If you are comfortable removing DWARF unwinding then I don't mind (it is not a terribly big project to add it back again).

@graydon
Copy link
Contributor Author

graydon commented Feb 6, 2013

@brson re-r? I've updated this with your comments (removing the crate flag, making all fns return bool, only checking it in the return-unwind case) and rebased forward to the present. It seems able to make check now with return_unwind turned on. This change does not force it on yet though. Just want to get most of the infrastructure merged. Will experiment a bit more before turning it on; different change.

@pcwalton
Copy link
Contributor

I'm in favor of having both strategies. I'm concerned about the fact that checking for failure is not zero-cost. In Servo I would like to not burden every function call with an extra check. Servo's performance is very gated on function calls for some workloads (DOM benchmarks). Every cycle counts.

@pcwalton
Copy link
Contributor

I did a benchmark of return-unwinding Ackermann versus no-return-unwinding Ackermann. Results:

No return unwinding:

real    0m2.026s
user    0m2.000s
sys 0m0.006s

Return unwinding:

real    0m7.874s
user    0m7.784s
sys 0m0.021s

Source is here: https://gist.github.com/pcwalton/4950363

Based on this I think I am opposed to switching off the DWARF-based unwinding. Return value unwinding should be used on platforms where DWARF-based unwinding is not supported.

This is similar to the way clang and gcc both support SjLj and DWARF-based unwinding, so it has a lot of precedent. It's also not very much code to maintain.

@ghost ghost assigned graydon May 2, 2013
@Aatch
Copy link
Contributor

Aatch commented Jun 5, 2013

We return immediate types directly, not in a return pointer, now. So this (from what I know) isn't feasible any more.

Either way, this is horribly bit-rotted so I'm going to close.

@Aatch Aatch closed this Jun 5, 2013
@graydon graydon removed their assignment Jun 16, 2014
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.

5 participants