Turn off debug assertions in benchmark configs; enable strings as steps based on cfg(test)

[andyleiserson]

Usually !debug_assertions => compact-gate, but that is not the case when profiling.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.23%. Comparing base (951bff3) to head (beca008).

❗ Current head beca008 differs from pull request most recent head c2cbeec. Consider uploading reports for the commit c2cbeec to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #963   +/-   ##
=======================================
  Coverage   89.23%   89.23%           
=======================================
  Files         158      158           
  Lines       21711    21718    +7     
=======================================
+ Hits        19374    19381    +7     
  Misses       2337     2337           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[akoshelev]

iirc we added debug assertions to catch issues related to invariant violations while running the benches. We had a case where oneshot ipa hanged and we couldn't figure out why.

I don't think removing this will cause any issues and we can always enable them manually in command line, just wanted to provide an explanation why it was there in the first place

[andyleiserson]

If there's a need to have an optimized build with stricter sanity checks, then maybe we should add a dedicated config for that (something like dev-opt)? Or perhaps reconsider the safety / performance tradeoff for the sanity checks that are having escapes, i.e. consider converting to a regular assert rather than a debug assert.

I've actually been mostly using the release config for benchmarking -- removing this bit of config doesn't have much direct impact on what I've been doing, but it does seem to me like the right thing to do. Having to edit Cargo.toml to make the bench config actually produce accurate benchmark results seems confusing.

The other change I had considered making was splitting the PRSS used index tracking to a separate feature flag. I can see how that would make sense if we want to use debug-assertions for "small but non-negligible performance cost" and a separate flag(s) for "very significant performance cost".

[akoshelev]

yea I think this change makes sense because we want to evaluate the performance. Just wanted to provide an explanation why it was added. It can always be enabled from command line, so I don't think it is an issue

[akoshelev]

I am not sure if this is right. We want descriptive gates to match compact gate structure in production code. This may enable us to use strings inside MPC circuits that then fail to compile with compact-gate enabled. Maybe we just want cfg[test] here?

[andyleiserson]

As I understand things (and possibly this can change with @martinthomson's new compact gate implementation), the compact gate supports only the steps that are part of the "production" IPA protocol. So anything that's not the production protocol needs descriptive gate. (Which is counter to my claim in my other comment that bench should work out-of-the-box for benchmarks -- but better to enable just descriptive gate, than descriptive gate plus every debug assertion.)

I agree that we don't want a coverage gap in CI for steps added to the protocol without compact gate support -- but doesn't the "compact gate tests" (cargo test --no-default-features --features "cli web-app real-world-infra test-fixture compact-gate") step in CI protect us from that? Not using a string as a step is necessary but not sufficient for a protocol to work with compact gate -- it also needs to be listed in steps.txt.

[akoshelev]

is there a strong reason why you want to use strings instead of enums outside of tests? I just feel like it introduces more friction working on the code where you compile everything just fine and rely on CI to catch those issues which delays the feedback loop unnecessarily.

[andyleiserson]

No, no good reason. I had been conflating strings as steps with descriptive gate in my head. It's totally reasonable to implement Step for benchmarks.

Your comment prompted me to wonder whether there's a compelling reason we need to support strings as steps at all... I took a quick look at this and it seems like maybe not. The list of places we do (post IPAv2 removal) is relatively short, and a bunch of those places we're narrowing the context unnecessarily, so the narrow can just be removed. Elsewhere we just need to add an enum with #[derive(Step)], which is a lot less work than when it required the enum definition plus a handful of impls for the enum.

[akoshelev]

I'd suggest to look again at using test flag to guard against using string steps in prod code, otherwise it is good to go

[andyleiserson]

I pushed a new version that defines real steps in the few places where a string-as-step appeared outside cfg(test).

(This makes the description of the PR, and the first commit, inaccurate.)

[andyleiserson]

Rebased to make the commit message accurate. Code is unchanged.