Reduce code bloat from conversion traits in function parameters #28256
Conversation
|
It would be nice if the compiler could do this automatically... with MIR, it should be possible to detect generic functions where only some small prefix depends on the type parameter. |
|
I would personally prefer to avoid this sort of manual conversion for now unless we've got some numbers saying otherwise. It looks like the binary sizes didn't change much, but could you also measure compile time as well? I expect that's the largest win to gain from conversions like this. I would also prefer a style that looked something like: fn f<Generic: AsRef<Concrete>>(arg: Generic) {
_f(arg.as_ref())
}
fn _f(arg: &Concrete) {
// ...
}I don't think the |
|
I also agree with @eefriedman that it'd be nice to have this automatically done in some cases, but I also wouldn't want to hold my breath for that to happen in the near future. I'd primarily like to see some measurements in terms of compile time to see what we can gain from a pattern like this. |
|
There is no real reason to add |
|
The difference in compile times is minimal: |
Doesn't
Sure, doing this manually is not optimal. Instead of compiler "magic" @aturon suggested explicit, but very consice syntax https://github.com/rust-lang/rfcs/blame/master/text/0529-conversion-traits.md#L271 which I like better. This patch is mostly trying to answer the question if this is worth doing at all.
Ok, I have no preferences here. |
|
LLVM should be smart enough to make inlining decisions. |
|
Without |
|
Without #[inline] + changes to "small" functions reverted I'd probably switch to this version. The size is a bit worse than in https://gist.github.com/petrochenkov/055e9df6c5260ac02a7d, but librustc_back, libterm, libstd and executables still benefit. |
|
Updated. |
|
@alexcrichton I'm happy to move forward with the current version of this code, but leave the final decision to you. |
This patch transforms functions of the form
```
fn f<Generic: AsRef<Concrete>>(arg: Generic) {
let arg: &Concrete = arg.as_ref();
// Code using arg
}
```
to the next form:
```
#[inline]
fn f<Generic: AsRef<Concrete>>(arg: Generic) {
fn f_inner(arg: &Concrete) {
// Code using arg
}
f_inner(arg.as_ref());
}
```
Therefore, most of the code is concrete and not duplicated during monomorphisation (unless inlined)
and only the tiny bit of conversion code is duplicated. This method was mentioned by @aturon in the
Conversion Traits RFC (https://github.com/rust-lang/rfcs/blame/master/text/0529-conversion-traits.md#L249) and similar techniques are not uncommon in C++ template libraries.
This patch goes to the extremes and applies the transformation even to smaller functions<sup>1</sup>
for purity of the experiment. *Some of them can be rolled back* if considered too ridiculous.
<sup>1</sup> However who knows how small are these functions are after inlining and everything.
The functions in question are mostly `fs`/`os` functions and not used especially often with variety
of argument types, so the code size reduction is rather small (but consistent). Here are the sizes
of stage2 artifacts before and after the patch:
https://gist.github.com/petrochenkov/e76a6b280f382da13c5d
https://gist.github.com/petrochenkov/6cc28727d5256dbdfed0
Note:
All the `inner` functions are concrete and unavailable for cross-crate inlining, some of them may
need `#[inline]` annotations in the future.
r? @aturon
This patch transforms functions of the form
to the next form:
Therefore, most of the code is concrete and not duplicated during monomorphisation (unless inlined)
and only the tiny bit of conversion code is duplicated. This method was mentioned by @aturon in the
Conversion Traits RFC (https://github.com/rust-lang/rfcs/blame/master/text/0529-conversion-traits.md#L249) and similar techniques are not uncommon in C++ template libraries.
This patch goes to the extremes and applies the transformation even to smaller functions1
for purity of the experiment. Some of them can be rolled back if considered too ridiculous.
1 However who knows how small are these functions are after inlining and everything.
The functions in question are mostly
fs/osfunctions and not used especially often with varietyof argument types, so the code size reduction is rather small (but consistent). Here are the sizes
of stage2 artifacts before and after the patch:
https://gist.github.com/petrochenkov/e76a6b280f382da13c5d
https://gist.github.com/petrochenkov/6cc28727d5256dbdfed0
Note:
All the
innerfunctions are concrete and unavailable for cross-crate inlining, some of them mayneed
#[inline]annotations in the future.r? @aturon