-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Return unwind #4342
Conversation
r+ |
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 |
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? |
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 |
@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.) |
I am in favor of having only one strategy. Even though long term I'd |
@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). |
@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. |
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. |
I did a benchmark of return-unwinding Ackermann versus no-return-unwinding Ackermann. Results: No return unwinding:
Return unwinding:
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. |
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. |
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.