Skip to content

refactor: Extract the cycle branches from fetch and maybe_changed_after#955

Merged
MichaReiser merged 2 commits into
salsa-rs:masterfrom
MichaReiser:extract-cycle-branches
Aug 6, 2025
Merged

refactor: Extract the cycle branches from fetch and maybe_changed_after#955
MichaReiser merged 2 commits into
salsa-rs:masterfrom
MichaReiser:extract-cycle-branches

Conversation

@MichaReiser

@MichaReiser MichaReiser commented Aug 2, 2025

Copy link
Copy Markdown
Contributor

and mark them as cold. I also reordered some other match statements so that the most likely branches come first.

This hopefully helps the compiler to generate more efficient code for the common case (no cycle)

@netlify

netlify Bot commented Aug 2, 2025

Copy link
Copy Markdown

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 8e0ee63
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6892f92e52efb50008b21e63

@MichaReiser MichaReiser changed the title Extract the cycle branches from fetch and maybe_changed_after refactor: Extract the cycle branches from fetch and maybe_changed_after Aug 2, 2025
@MichaReiser MichaReiser force-pushed the extract-cycle-branches branch from b19ec4f to d75390e Compare August 2, 2025 20:37
@codspeed-hq

codspeed-hq Bot commented Aug 2, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #955 will not alter performance

Comparing MichaReiser:extract-cycle-branches (8e0ee63) with master (d66fe33)

Summary

✅ 12 untouched benchmarks

@MichaReiser MichaReiser requested a review from ibraheemdev August 2, 2025 20:56
@MichaReiser MichaReiser force-pushed the extract-cycle-branches branch from d75390e to 3bdf4ba Compare August 3, 2025 07:57
@MichaReiser MichaReiser requested a review from carljm August 5, 2025 16:16

@carljm carljm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

}
}

QueryOriginRef::Assigned(_) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is just to put the more frequent match arm first?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I find this also helps with readability because I can reason about the common case first

Comment thread src/function/fetch.rs
@MichaReiser MichaReiser force-pushed the extract-cycle-branches branch from 3bdf4ba to 8e0ee63 Compare August 6, 2025 06:41
@MichaReiser MichaReiser enabled auto-merge August 6, 2025 06:45
@MichaReiser MichaReiser added this pull request to the merge queue Aug 6, 2025
Merged via the queue into salsa-rs:master with commit 5b2a97b Aug 6, 2025
12 checks passed
@MichaReiser MichaReiser deleted the extract-cycle-branches branch August 6, 2025 06:59
@github-actions github-actions Bot mentioned this pull request Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants