Skip to content

RangeInclusive performance regression in beta + nightly #119643

Closed
@conradludgate

Description

@conradludgate
Contributor

Code

I tried this code:

https://github.com/conradludgate/iter-num-tools/blob/024f521782cd241dea81d031cf802f22fc66bab9/benches/gridspace.rs

grid_space([1.0, 1.0]..=[100.0, 100.0], 200)

I expected to see this happen: Benchmark results to stay consistent between versions

Instead, this happened:

$ cargo +stable bench GridSpace -q
GridSpace/gridspace [1.0, 100.0] x200 (iter-num-tools)
                        time:   [114.03 µs 114.55 µs 115.19 µs]

$ cargo +beta bench GridSpace -q
GridSpace/gridspace [1.0, 100.0] x200 (iter-num-tools)
                        time:   [159.80 µs 172.22 µs 184.65 µs]
                        change: [+54.390% +64.190% +75.784%] (p = 0.00 < 0.05)
                        Performance has regressed.

$ cargo +nightly bench GridSpace -q
GridSpace/gridspace [1.0, 100.0] x200 (iter-num-tools)
                        time:   [140.26 µs 142.47 µs 144.66 µs]
                        change: [-30.426% -25.917% -20.649%] (p = 0.00 < 0.05)
                        Performance has improved.

Version it worked on

It most recently worked on:

rustc 1.75.0 (82e1608df 2023-12-21)
binary: rustc
commit-hash: 82e1608dfa6e0b5569232559e3d385fea5a93112
commit-date: 2023-12-21
host: aarch64-apple-darwin
release: 1.75.0
LLVM version: 17.0.6

Version with regression

rustc +beta --version --verbose:

rustc 1.76.0-beta.1 (0e09125c6 2023-12-21)
binary: rustc
commit-hash: 0e09125c6c3c2fd70d7de961bcf0e51575235fad
commit-date: 2023-12-21
host: aarch64-apple-darwin
release: 1.76.0-beta.1
LLVM version: 17.0.6

rustc +nightly --version --verbose:

rustc 1.77.0-nightly (f688dd684 2024-01-04)
binary: rustc
commit-hash: f688dd684faca5b31b156fac2c6e0ae81fc9bc90
commit-date: 2024-01-04
host: aarch64-apple-darwin
release: 1.77.0-nightly
LLVM version: 17.0.6

More info

I haven't been able to fully track down the regression, but it has something to do with RangeInclusive. Changing the benchmark to grid_space([1.0, 1.0]..[100.0, 100.0], 200) shows much less difference between versions. So far, the smallest I have reduced the repro is https://gist.github.com/conradludgate/b604d1ab0898df3babd80e384377c2b2

Activity

added
I-prioritizeIssue: Indicates that prioritization has been requested for this issue.
needs-triageThis issue may need triage. Remove it if it has been sufficiently triaged.
on Jan 5, 2024
conradludgate

conradludgate commented on Jan 5, 2024

@conradludgate
ContributorAuthor

Ok, this seems to be a missed-opt compiler regression. replacing

b.iter(|| {
    let lerp = Linear {
        interpolate: LinearInterpolation {
            start: 1.0,
            step: 1.0,
        },
        length: 200,
    };
    let grid = GridSpaceInterpolation([lerp, lerp]);
    let space = Space {
        interpolate: grid,
        range: 1..=40000,
    };
    bench(space);
})

with

b.iter(f)

fn f() {
    let lerp = Linear {
        interpolate: LinearInterpolation {
            start: 1.0,
            step: 1.0,
        },
        length: 200,
    };
    let grid = GridSpaceInterpolation([lerp, lerp]);
    let space = Space {
        interpolate: grid,
        range: 1..=40000,
    };
    bench(space);
}

removed the perf regression.

perhaps it's a missed const-prop. I can't tell any noticeable difference in the generated assembly though

deltragon

deltragon commented on Jan 5, 2024

@deltragon
Contributor

There was recently some changes to const prop in #116012, which also did appear to regress some codegen tests - those change back with #111344.

conradludgate

conradludgate commented on Jan 5, 2024

@conradludgate
ContributorAuthor

Thanks, I'll test with that branch

conradludgate

conradludgate commented on Jan 5, 2024

@conradludgate
ContributorAuthor

It does indeed look like #111344 does resolve this regression

$ cargo +stage2 bench GridSpace -q
GridSpace/gridspace [1.0, 100.0] x200 (iter-num-tools)
                        time:   [118.57 µs 119.86 µs 120.98 µs]
                        change: [-39.166% -34.990% -30.236%] (p = 0.00 < 0.05)
                        Performance has improved.
added
A-mir-optArea: MIR optimizations
A-const-propArea: Constant propagation
and removed
needs-triageThis issue may need triage. Remove it if it has been sufficiently triaged.
on Jan 6, 2024
apiraino

apiraino commented on Jan 7, 2024

@apiraino
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

added and removed
I-prioritizeIssue: Indicates that prioritization has been requested for this issue.
on Jan 7, 2024
deltragon

deltragon commented on Jan 17, 2024

@deltragon
Contributor

#119670 has been merged, which recovers some regressions from #116012. Can you test if the issue is fixed on latest master?

conradludgate

conradludgate commented on Jan 26, 2024

@conradludgate
ContributorAuthor

Fixed on latest nightly, thanks

mati865

mati865 commented on Jan 26, 2024

@mati865
Member

It's fixed only on nightly though, does anyone know what is the policy for backporting performance fixes?

added
beta-nominatedNominated for backporting to the compiler in the beta channel.
on Jan 26, 2024
ChrisDenton

ChrisDenton commented on Jan 26, 2024

@ChrisDenton
Member

I've added beta-nominated in the hopes of finding out if backporting this perf fix to beta is acceptable.

apiraino

apiraino commented on Jan 31, 2024

@apiraino
Contributor

@ChrisDenton this is an issue. I see a number of PRs about solving this issue. I think #119670 is the object of your backport request, am I right? Can you help me out? 🙂 Thanks!

removed
beta-nominatedNominated for backporting to the compiler in the beta channel.
on Jan 31, 2024
ChrisDenton

ChrisDenton commented on Jan 31, 2024

@ChrisDenton
Member

Oh no, sorry! I totally spaced on that. I'll add to the PR>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-const-propArea: Constant propagationA-mir-optArea: MIR optimizationsC-bugCategory: This is a bug.P-mediumMedium priorityregression-untriagedUntriaged performance or correctness regression.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @mati865@ChrisDenton@apiraino@conradludgate@deltragon

        Issue actions

          `RangeInclusive` performance regression in beta + nightly · Issue #119643 · rust-lang/rust