Remove unnecessary abstractions from option and result impls

[tmiasko]

Change the implementation of a option and result methods
to avoid code generation of additional functions, where
a simple match is sufficient.

[rust-highfive]

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

[sdroege]

FWIW there were a couple of PRs recently that did the exact opposite (inside the compiler), for example #81084

[tmiasko]

Changes in the standard library reduce the amount of necessary codegen for everyone using the standard library, and for each instantiation in each code generation unit (when method is marked inline).

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 0e1bc1f0a89e5257063c2c4cf2df417ced15c86f with merge 8af8326194c1c86d79c90ab07f97bd831dda7c57...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8af8326194c1c86d79c90ab07f97bd831dda7c57 (8af8326194c1c86d79c90ab07f97bd831dda7c57)

[rust-timer]

Queued 8af8326194c1c86d79c90ab07f97bd831dda7c57 with parent e708cbd, future comparison URL.

[sdroege]

Changes in the standard library reduce the amount of necessary codegen for everyone using the standard library, and for each instantiation in each code generation unit (when method is marked inline).

Yeah my point was mostly to make sure that this is not ping-ponged between the two variants, and that the costs of the combinator functions is clear (possible clearer code vs. compilation time).

[rust-timer]

Finished benchmarking try commit (8af8326194c1c86d79c90ab07f97bd831dda7c57): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[Julian-Wollersberger]

Changes in the standard library reduce the amount of necessary codegen for everyone using the standard library, and for each instantiation in each code generation unit (when method is marked inline).

It's important to actually measure the impact on llvm-lines, because the results are very often surprising.
See this chaper from the dev guide on how to that.

I did that, and the results are negligible. The standard library went from 338978 total llvm lines up to 339051. That's +73.
So this PR has no effect on the amount of work that is shoved to LLVM.

Personally, I still like these changes, since it makes the implementation easier to understand.
It might have a runtime performance effect in debug builds (because no inlining is done in debug mode), but I can't measure that right now.

Commands, for reference:

cargo install cargo-llvm-lines
# check out this PR's branch
RUSTFLAGS="--emit=llvm-ir" CARGOFLAGS="-Ztimings" ./x.py clean
RUSTFLAGS="--emit=llvm-ir" CARGOFLAGS="-Ztimings" ./x.py build --stage 0 library/std
cargo llvm-lines --files ./build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/*.ll > llvm-lines-std-before.txt

# Check out the commit before that
RUSTFLAGS="--emit=llvm-ir" CARGOFLAGS="-Ztimings" ./x.py clean
RUSTFLAGS="--emit=llvm-ir" CARGOFLAGS="-Ztimings" ./x.py build --stage 0 library/std
cargo llvm-lines --files ./build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/*.ll > llvm-lines-std-after.txt

[tmiasko]

Wow, thanks for checking that.

The --emit=llvm-ir represents the LLVM IR after optimizations. If you are interested in the input, I would suggest examining the input bitcode obtained from -Csave-temps.

Alas, using your methodology I obtained a reduction of 65 lines (355936 before, and 355871 after). I repeated measurement twice to check if the numbers are reproducible and they are. Though, as above, I don't think those numbers are meaningful.

BTW. Those changes are about functions like copied, as_deref, and as_deref_mut, yes they have negligible impact in context of the whole standard library.

EDIT: Using no-opt.bc, I measured a reduction of 1357 lines (1209870 before, 1208513 after).

Commands for reference:

env RUSTFLAGS="-Csave-temps" ./x.py clean
env RUSTFLAGS="-Csave-temps" ./x.py build --stage 0 library/std
for f in ./build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/*.no-opt.bc; do
  ./build/x86_64-unknown-linux-gnu/llvm/bin/llvm-dis "$f";
done
cargo-llvm-lines llvm-lines --files ./build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/*.ll

[panstromek]

OT but catched my eye.

The --emit=llvm-ir represents the LLVM IR after optimizations.

Are you sure this is the case? Is it written somewhere? I haven't found it mentioned in the docs, but it could explain a lot of confusing behaviour I (and others) encountered when using cargo-llvm-lines.

If that's the case, would it make sense to update cargo-llvm-lines to use the method with disassembling bitcode from temps you mentioned instead? Is that reliable enough to always work? The tool currently uses just --emit=llvm-ir option, which can be misleading if the IR is already optimized. The other way would be to somehow force rustc do no optimization at all, which I am not sure is possible (afaik, some optimizatins are run in debug mode, too).

[tmiasko]

Are you sure this is the case? Is it written somewhere? I haven't found it mentioned in the docs, but it could explain a lot of confusing behaviour I (and others) encountered when using cargo-llvm-lines.

Yes. With a caveat that cargo-llvm-lines in a context of a cargo project uses -Cno-prepopulate-passes to create an empty optimization pipeline. In contrast, the commands #81747 (comment) didn't include this flag, and cargo-llvm-lines was used only to postprocess produced LLVM IR.

[panstromek]

Thanks, that makes sense. I forgot about -Cno-prepopulate-passes. I just observe wildly different llvm-lines results for release & debug builds, so I am trying to figure out if that can be caused just by overflow checks and debug assertions or if there's something else in play.

[JohnCSimon]

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[tmiasko]

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[Dylan-DPC-zz]

r? @m-ou-se

[m-ou-se]

Considering rust-timer didn't show any improvement and the llvm lines also barely changed, what's the remaining motivation for this change?

[tmiasko]

While rebasing I also noticed that one of methods had been changed already, since in the proposed form it can be easier to turn it into a const fn. Otherwise, at this point, I think it mostly awaits decision on whether to merge it or not. Thanks.

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[m-ou-se]

In that case I'm closing this. Of all of the changes, I only think the one for flatten is an improvement. Feel free to send a PR to change that one.