Skip to content

perf(semantic): use split_at_mut instead of iterator in current_and_parent_mut#17182

Merged
graphite-app[bot] merged 1 commit intomainfrom
c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut
Dec 22, 2025
Merged

perf(semantic): use split_at_mut instead of iterator in current_and_parent_mut#17182
graphite-app[bot] merged 1 commit intomainfrom
c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented Dec 20, 2025

Before:

impl Stack {
    #[inline]
    pub fn current_and_parent_iter(&mut self) -> (&mut Map, &mut Map) {
        // Assert invariants
        if self.depth == 0 {
            unsafe { unreachable_unchecked() };
        }
        if self.stack.len() <= self.depth {
            unsafe { unreachable_unchecked() };
        }
        let mut iter = self.stack.iter_mut();
        let parent = iter.nth(self.depth - 1).unwrap();
        let current = iter.next().unwrap();
        (current, parent)
    }
}

pub fn use_iter(s: &mut Stack) -> (&mut Map, &mut Map) {
    s.current_and_parent_iter()
}
use_iter:
        mov     rax, qword ptr [rdi + 24]
        movabs  rcx, 1152921504606846975
        and     rcx, rax
        cmp     rcx, qword ptr [rdi + 16]
        je      .LBB0_2
        lea     rcx, [rax + 2*rax]
        shl     rcx, 4
        mov     rdx, qword ptr [rdi + 8]
        lea     rax, [rdx + rcx]
        add     rdx, rcx
        add     rdx, -48
        ret
.LBB0_2:
        push    rax
        lea     rdi, [rip + .Lanon.f6daf3c64be0965473887c7779875594.1]
        call    qword ptr [rip + core::option::unwrap_failed::hc7ed8ec7fd6c106d@GOTPCREL]

.Lanon.f6daf3c64be0965473887c7779875594.0:
        .asciz  "/app/example.rs"

.Lanon.f6daf3c64be0965473887c7779875594.1:
        .quad   .Lanon.f6daf3c64be0965473887c7779875594.0
        .asciz  "\017\000\000\000\000\000\000\000\025\000\000\000#\000\000"

After:

impl Stack {
    #[inline]
    pub fn current_and_parent_split(&mut self) -> (&mut Map, &mut Map) {
        // Assert invariants
        if self.depth == 0 {
            unsafe { unreachable_unchecked() };
        }
        if self.stack.len() <= self.depth {
            unsafe { unreachable_unchecked() };
        }
        let (head, tail) = self.stack.split_at_mut(self.depth);
        let parent = &mut head[self.depth - 1] ;
        let current = &mut tail[0];
        (current, parent)
    }
}

pub fn use_split(s: &mut Stack) -> (&mut Map, &mut Map) {
    s.current_and_parent_split()
}
use_split:
        mov     rcx, qword ptr [rdi + 8]
        mov     rax, qword ptr [rdi + 24]
        lea     rdx, [rax + 2*rax]
        shl     rdx, 4
        lea     rax, [rcx + rdx]
        add     rdx, rcx
        add     rdx, -48
        ret

@github-actions github-actions bot added the A-semantic Area - Semantic label Dec 20, 2025
Copy link
Contributor Author

camc314 commented Dec 20, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Dec 20, 2025
@camc314 camc314 force-pushed the c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut branch from 62857fc to 32608c4 Compare December 20, 2025 15:23
@camc314 camc314 self-assigned this Dec 20, 2025
@camc314 camc314 force-pushed the c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut branch from 32608c4 to 18ed7de Compare December 20, 2025 15:24
@camc314 camc314 changed the title perf(semantic): use split_at_mut instead of iterator in current_and_parent_mut perf(semantic): use split_at_mut instead of iterator in current_and_parent_mut Dec 20, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 20, 2025

CodSpeed Performance Report

Merging #17182 will not alter performance

Comparing c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut (18ed7de) with main (0f63e75)

Summary

✅ 42 untouched
⏩ 3 skipped1

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@camc314 camc314 marked this pull request as ready for review December 20, 2025 15:32
@camc314 camc314 requested a review from Dunqing as a code owner December 20, 2025 15:32
Copilot AI review requested due to automatic review settings December 20, 2025 15:32
@camc314 camc314 assigned overlookmotel and unassigned camc314 Dec 20, 2025
@camc314
Copy link
Contributor Author

camc314 commented Dec 20, 2025

😞 I was more optimistic that there'd be a more concrete perf improvement.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes the current_and_parent_mut method in the unresolved references stack by replacing an iterator-based approach with split_at_mut, resulting in cleaner assembly code with fewer instructions and no panic paths.

Key Changes

  • Replaced iter_mut().nth().next() pattern with split_at_mut() for better code generation
  • Removed outdated godbolt.org link comment since the optimization approach has changed
  • Maintained the same safety guarantees through existing assert_unchecked! calls

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@overlookmotel
Copy link
Member

Weird. It's regressed in since Rust 1.79. Used not to have a panicking branch.

You got a Godbolt link for new vs old? (just checking this isn't Claude inventing assembly again)

It's also useful to put a Godbolt link in a comment above code like this, which we can use to verify the assembly is still what it should be.

@camc314
Copy link
Contributor Author

camc314 commented Dec 21, 2025

Yep i did verify these as claude can hallicinate here:

https://godbolt.org/z/r55YzaoPq

@camc314
Copy link
Contributor Author

camc314 commented Dec 21, 2025

I made an issue here rust-lang/rust#150235 it's a regression in 1.82

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 22, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 22, 2025

Merge activity

…d_parent_mut` (#17182)

Before:

```rs
impl Stack {
    #[inline]
    pub fn current_and_parent_iter(&mut self) -> (&mut Map, &mut Map) {
        // Assert invariants
        if self.depth == 0 {
            unsafe { unreachable_unchecked() };
        }
        if self.stack.len() <= self.depth {
            unsafe { unreachable_unchecked() };
        }
        let mut iter = self.stack.iter_mut();
        let parent = iter.nth(self.depth - 1).unwrap();
        let current = iter.next().unwrap();
        (current, parent)
    }
}

pub fn use_iter(s: &mut Stack) -> (&mut Map, &mut Map) {
    s.current_and_parent_iter()
}
```

```asm
use_iter:
        mov     rax, qword ptr [rdi + 24]
        movabs  rcx, 1152921504606846975
        and     rcx, rax
        cmp     rcx, qword ptr [rdi + 16]
        je      .LBB0_2
        lea     rcx, [rax + 2*rax]
        shl     rcx, 4
        mov     rdx, qword ptr [rdi + 8]
        lea     rax, [rdx + rcx]
        add     rdx, rcx
        add     rdx, -48
        ret
.LBB0_2:
        push    rax
        lea     rdi, [rip + .Lanon.f6daf3c64be0965473887c7779875594.1]
        call    qword ptr [rip + core::option::unwrap_failed::hc7ed8ec7fd6c106d@GOTPCREL]

.Lanon.f6daf3c64be0965473887c7779875594.0:
        .asciz  "/app/example.rs"

.Lanon.f6daf3c64be0965473887c7779875594.1:
        .quad   .Lanon.f6daf3c64be0965473887c7779875594.0
        .asciz  "\017\000\000\000\000\000\000\000\025\000\000\000#\000\000"
```

After:

```rs
impl Stack {
    #[inline]
    pub fn current_and_parent_split(&mut self) -> (&mut Map, &mut Map) {
        // Assert invariants
        if self.depth == 0 {
            unsafe { unreachable_unchecked() };
        }
        if self.stack.len() <= self.depth {
            unsafe { unreachable_unchecked() };
        }
        let (head, tail) = self.stack.split_at_mut(self.depth);
        let parent = &mut head[self.depth - 1] ;
        let current = &mut tail[0];
        (current, parent)
    }
}

pub fn use_split(s: &mut Stack) -> (&mut Map, &mut Map) {
    s.current_and_parent_split()
}
```

```asm
use_split:
        mov     rcx, qword ptr [rdi + 8]
        mov     rax, qword ptr [rdi + 24]
        lea     rdx, [rax + 2*rax]
        shl     rdx, 4
        lea     rax, [rcx + rdx]
        add     rdx, rcx
        add     rdx, -48
        ret
```
@graphite-app graphite-app bot force-pushed the c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut branch from 18ed7de to 315c9ed Compare December 22, 2025 13:50
@graphite-app graphite-app bot merged commit 315c9ed into main Dec 22, 2025
20 checks passed
@graphite-app graphite-app bot deleted the c/12-20-perf_semantic_use_split_at_mut_instead_of_iterator_in_current_and_parent_mut branch December 22, 2025 13:56
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants