Add custom profile for debug builds with basic optimizations

[thedataking]

Regular debug builds need to be debuggable according to the principle of least surprise. This is also expected by tools.

There is a need for optimized builds with assertions enabled so we add that as a custom profile instead.

[kkysen]

It's very annoying that the debug profile is actually named dev, as opt-level = 1 does make sense for a dev profile, as that's what we want to use most often for development, but it doesn't make sense for debugging. Plus that means we can't use the profile names debug or dev for anything else.

Furthermore, I think there are also some other distinctions. How about this?

# For debugging.
#
# This enables no optimizations, so optimizations can't interfere with debugging,
# but it will probably be very slow.
[profile.dev]

# Fastest for local development.
#
# `debug`/`dev` builds are very slow*, so this enables some optimizations
# to be fastest for the local development cycle of compiling and running tests.
# Only some optimizations are enabled, so compile time doesn't increase very much.
# Full debug info is still enabled, so this can also be used for debugging,
# but there are still some cases where optimizations can still interfere with debugging.
#
# * Especially as we increasingly use more zero-cost abstractions,
# which are only zero-cost with optimizations.
[profile.fast-dev]
inherits = "dev"
opt-level = 1

# Fastest.
# For release builds.
[profile.release]
codegen-units = 1

# Fast but checked.
# For CI and sometimes local development with qemu.
#
# That is, it compiles with all runtime checks and limited debug info (as opposed to none).
[profile.checked-release]
inherits = "release"
opt-level = 3
debug = "limited"
debug-assertions = true
overflow-checks = true
lto = "thin"
codegen-units = 16

Using these

• dev/debug is used for gdb-style debugging
• fast-dev is used locally for development by default
• release is used for perf profiling and actual releases
• checked-release is used for CI tests (this should also make CI much faster but still have the same amount of checks). No more duplicated release and debug CI tests.

[thedataking]

I opened this PR to quickly resolve a concrete debugging issue and was hoping for a quick yes/no approval. I was hoping that I could be of assistance so you could focus on high priority items such as obu parsing.

The default and release profiles should not need customization (codegen-units = 1 is only okay temporarily). If you have any strong objections to the current PR, I will address them as best I can, otherwise I'll merge as-is.

Some general principles:

• Relying on defaults is ideal as other developers already understand them and expect them to behave in a certain way
  • example: I was surprised debug profile wasn't working, after this PR it works as expected again
  • implication: no need to comment on [profile.dev]
  • implication: we should eventually get rid of codegen-units = 1 on release (I understand why it is temporarily there)
• Source code is long lived, this project could be used for a decade or more
  • implication: we shouldn't comment on short term concerns such as how compile-times evolve, imagine yourself reading your comments in 10 years, what is still relevant (and surprising) and what is overcome by events?
• Done is better than perfect. Time spent here is time we don't spend elsewhere (e.g. removing unsafety).
  • implication: we need to pick our battles wisely; some things will be less than ideal, some things we can get back to. Use issues for those, spend 95%+ of your time on the "burning fires".
  • suggestion: ask yourself – if the project ended abruptly (e.g. end of this year), what are the things you would absolutely make sure would get done? (e.g. making functions safe, fixing the core data structures). What things could we ultimately live without under extreme time pressure? (this PR would be one of those things.)
  • fact: we are facing time pressure.

If you want to talk about any of the general principles, happy to do so on the dev dojo, this PR and similar papercuts should just be quickly resolved as fast as we can.

[rinon]

This particular change (given the context of what the goal was) LGTM as is. I like the idea of a check-release profile, but that can be a separate PR at some future point, it doesn't need to happen right now to unblock debugger usage. When we have a concrete need for more improvements that's ok with me. Happy to discuss further offline, but I don't think we should spend much time here.

[kkysen]

This is good for now. Only other comment is, should

rav1d/tests/meson.build

Line 156 in 2a27419

if get_option('debug')

be modified at all in accordance (I'd rather remove that whole meson section that runs cargo; it hides the output and I never use and neither does CI)?

[kkysen]

I agree further improvements can wait until later with issues filed instead so we can focus on the more important parts, though I think that applies equally to this, too (that this PR should've just been an issue for now, since it takes time to review, etc., though it's fine now that it's written and reviewed).

The default and release profiles should not need customization

In general, I don't agree with this. This happens often. For example, people will often compile dependencies with full optimizations in debug mode since they rarely change and can make debugging/development painfully slow. It's like how Phillip recommended using a release build with assertion enabled instead of a full debug build. And I've seen people change the debug optimization level for this reason for the main crate before, too. As for the release profile, extra optimizations are fairly common for binaries. Note that all of these only apply to the binaries; users of the library will do whatever they want and it ignores all of our profiles. So these changes are just for our use developing, testing, debugging, and profiling.

Also I don't think a few extra comments are detrimental. The dev/debug profile name is confusing. If cargo were rewritten/had breaking changes, I think this would be fixed. So clarifying that one profile is for debugging but is suboptimal for normal development where you run tests I don't think is bad. If we moved the meson tests into cargo test, perhaps this could be done under profile.test instead.

[thedataking]

this PR should've just been an issue for now, since it takes time to review, etc

yup.

I'd rather remove that whole meson section that runs cargo

removed; it wasn't even done right -.-

I adjusted the expected paths so now we can just run

.github/workflows/test.sh -r target/release/dav1d -s target/release/seek_stress

from the top rav1d dir. It was annoying me not being able to tab-complete the paths ...

[kkysen]

@Perl, this somehow broke local builds (i.e., cargo build). cargo build --profile opt-dev works, though. I tried from a fresh git clone, too.

  = note: /usr/bin/ld: /home/kkysen/work/rust/rav1d2/target/debug/deps/librav1d-5d0eefa0b0ef5b4f.rlib(rav1d-5d0eefa0b0ef5b4f.4djp0c4y40q5tr3z.rcgu.o): in function `rav1d::src::ipred::intra_pred_dsp_init_x86':
          /home/kkysen/work/rust/rav1d2/src/ipred.rs:2152: undefined reference to `dav1d_ipred_dc_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2154: undefined reference to `dav1d_ipred_dc_128_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2156: undefined reference to `dav1d_ipred_dc_top_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2158: undefined reference to `dav1d_ipred_dc_left_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2160: undefined reference to `dav1d_ipred_h_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2162: undefined reference to `dav1d_ipred_v_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/target/debug/deps/librav1d-5d0eefa0b0ef5b4f.rlib(rav1d-5d0eefa0b0ef5b4f.4djp0c4y40q5tr3z.rcgu.o): in function `rav1d::src::ipred::intra_pred_dsp_init_x86':
          /home/kkysen/work/rust/rav1d2/src/ipred.rs:2152: undefined reference to `dav1d_ipred_dc_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2154: undefined reference to `dav1d_ipred_dc_128_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2156: undefined reference to `dav1d_ipred_dc_top_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2158: undefined reference to `dav1d_ipred_dc_left_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2160: undefined reference to `dav1d_ipred_h_16bpc_avx512icl'
          /usr/bin/ld: /home/kkysen/work/rust/rav1d2/src/ipred.rs:2162: undefined reference to `dav1d_ipred_v_16bpc_avx512icl'
          collect2: error: ld returned 1 exit status