Avoid double indirection for the "self" arg in methods #7452
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Didn't someone just land the exact opposite of this change? |
|
You're probably referring to #7410 by @luqmana which made "self" (without any sigil) be passed by reference. That's the one I keep as "by reference", as I mentioned in the commit/PR message. Though I did revert a few changes from that commit that were required to tell the self and &@~self apart. Edit: @luqmana did that, not @msullivan. |
"self" is always passed as an opaque box, so there's no point in using the concrete self type when translating the argument. All it does it causing the value to be casted back to an opaque box right away.
The code that tried to revoke the cleanup for the self argument tried to use "llself" to do so, but the cleanup might actually be registered with a different ValueRef due to e.g. casting. Currently, this is worked around by early revocation of the cleanup for self in trans_self_arg. To handle this correctly, we have to put the ValueRef for the cleanup into the MethodData, so trans_call_inner can use it to revoke the cleanup when it's actually supposed to.
Currently we pass all "self" arguments by reference, for the pointer variants this means that we end up with double indirection which causes a unnecessary performance hit. The fix itself is pretty straight-forward and just means that "self" needs to be handled like any other argument, except for by-value "self" which still needs to be passed by reference. This is because non-pointer types can't just be stuffed into the environment slot which is used to pass "self". What made things tricky is that there was also a bug in the typechecker where the method map entries are created. For type impls, that stored the base type instead of the actual self-type in the method map, e.g. Foo instead of &Foo for &self. That worked with pass-by-reference, but fails with pass-by-value which needs the real type. Code that makes use of methods seems to be about 10% faster with this change. Also, build times are reduced by about 4%. Fixes rust-lang#4355, rust-lang#4402, rust-lang#5280, rust-lang#4406 and rust-lang#7285
|
Can't reproduce this failure locally, neither with x86_64 nor with a i686 cross compile. Interestingly, bors didn't add a link to the log of the test run that actually failed, which is http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/174/steps/test/logs/stdio. Any ideas? |
bors
added a commit
that referenced
this pull request
Jun 29, 2013
Currently we pass all "self" arguments by reference, for the pointer variants this means that we end up with double indirection which causes a unnecessary performance hit. The fix itself is pretty straight-forward and just means that "self" needs to be handled like any other argument, except for by-value "self" which still needs to be passed by reference. This is because non-pointer types can't just be stuffed into the environment slot which is used to pass "self". What made things tricky is that there was also a bug in the typechecker where the method map entries are created. For type impls, that stored the base type instead of the actual self-type in the method map, e.g. Foo instead of &Foo for &self. That worked with pass-by-reference, but fails with pass-by-value which needs the real type. Code that makes use of methods seems to be about 10% faster with this change. Also, build times are reduced by about 4%. Fixes #4355, #4402, #5280, #4406 and #7285
This was referenced Jul 10, 2013
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jul 18, 2022
…ffate Fixes for `branches_sharing_code` fixes rust-lang#7198 fixes rust-lang#7452 fixes rust-lang#7555 fixes rust-lang#7589 changelog: Don't suggest moving modifications to locals used in any of the condition expressions in `branches_sharing_code` changelog: Don't suggest moving anything after a local with a significant drop in `branches_sharing_code`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we pass all "self" arguments by reference, for the pointer
variants this means that we end up with double indirection which causes
a unnecessary performance hit.
The fix itself is pretty straight-forward and just means that "self"
needs to be handled like any other argument, except for by-value "self"
which still needs to be passed by reference. This is because
non-pointer types can't just be stuffed into the environment slot which
is used to pass "self".
What made things tricky is that there was also a bug in the typechecker
where the method map entries are created. For type impls, that stored
the base type instead of the actual self-type in the method map, e.g.
Foo instead of &Foo for &self. That worked with pass-by-reference, but
fails with pass-by-value which needs the real type.
Code that makes use of methods seems to be about 10% faster with this
change. Also, build times are reduced by about 4%.
Fixes #4355, #4402, #5280, #4406 and #7285