Skip to content

perf!: remove bound-caching from Type, do in SumType::General instead#3022

Open
acl-cqc wants to merge 6 commits intomainfrom
acl/boundless_type
Open

perf!: remove bound-caching from Type, do in SumType::General instead#3022
acl-cqc wants to merge 6 commits intomainfrom
acl/boundless_type

Conversation

@acl-cqc
Copy link
Copy Markdown
Contributor

@acl-cqc acl-cqc commented Apr 10, 2026

Type currently caches the TypeBound alongside its Term; there are only three possible Term:: variants, and the cache is pretty much redundant in two (and a half):

  • Term::RuntimeFunction --> we know it's TypeBound::Copyable
  • Term::RuntimeExtension --> the ExtensionType also stores the bound, so we can just read that
  • Term::RuntimeSum depends on the SumType
    • SumType::Unary --> we know it's TypeBound::Copyable
    • SumType::General --> the only case where we actually need it

Hence, make SumType::General store a struct GeneralSum, with bound and rows (these are kept private to avoid mutating rows without updating the bound). And remove the cache in Type.

BREAKING CHANGE: any match on SumType::General {rows} ...rows... becomes SumType::General(gs) ...gs.rows()... ; construct GeneralSum with ::new

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 10, 2026

Merging this PR will improve performance by 17.69%

⚡ 1 improved benchmark
✅ 28 untouched benchmarks
⏩ 6 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
construction 20.2 µs 17.2 µs +17.69%

Comparing acl/boundless_type (417c6eb) with main (d372743)

Open in CodSpeed

Footnotes

  1. 6 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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 95.38462% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.09%. Comparing base (d372743) to head (417c6eb).

Files with missing lines Patch % Lines
hugr-core/src/types.rs 93.02% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3022   +/-   ##
=======================================
  Coverage   81.09%   81.09%           
=======================================
  Files         241      241           
  Lines       45054    45060    +6     
  Branches    38822    38828    +6     
=======================================
+ Hits        36536    36541    +5     
  Misses       6542     6542           
- Partials     1976     1977    +1     
Flag Coverage Δ
python 88.89% <ø> (ø)
rust 79.84% <95.38%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@acl-cqc acl-cqc changed the base branch from main to acl/type_wraps_term April 10, 2026 17:09
Comment thread hugr-core/src/types.rs
SumType::General(general) => {
let changed = general.rows.transform(tr)?;
if changed {
*general = GeneralSum::new(std::mem::take(&mut general.rows));
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.

Does this need a comment to say, recompute the bound?

@hugrbot
Copy link
Copy Markdown
Collaborator

hugrbot commented Apr 10, 2026

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary
    Building hugr v0.27.1 (current)
     Built [  35.089s] (current)
   Parsing hugr v0.27.1 (current)
    Parsed [   0.004s] (current)
  Building hugr v0.27.1 (baseline)
     Built [  34.568s] (baseline)
   Parsing hugr v0.27.1 (baseline)
    Parsed [   0.004s] (baseline)
  Checking hugr v0.27.1 -> v0.27.1 (assume minor change)
   Checked [   0.012s] 196 checks: 196 pass, 56 skip
   Summary no semver update required
  Finished [  72.609s] hugr
  Building hugr-cli v0.27.1 (current)
     Built [  29.186s] (current)
   Parsing hugr-cli v0.27.1 (current)
    Parsed [   0.006s] (current)
  Building hugr-cli v0.27.1 (baseline)
     Built [  28.788s] (baseline)
   Parsing hugr-cli v0.27.1 (baseline)
    Parsed [   0.006s] (baseline)
  Checking hugr-cli v0.27.1 -> v0.27.1 (assume minor change)
   Checked [   0.015s] 196 checks: 196 pass, 56 skip
   Summary no semver update required
  Finished [  59.668s] hugr-cli
  Building hugr-core v0.27.1 (current)
     Built [  24.901s] (current)
   Parsing hugr-core v0.27.1 (current)
    Parsed [   0.086s] (current)
  Building hugr-core v0.27.1 (baseline)
     Built [  24.797s] (baseline)
   Parsing hugr-core v0.27.1 (baseline)
    Parsed [   0.078s] (baseline)
  Checking hugr-core v0.27.1 -> v0.27.1 (assume minor change)
   Checked [   0.238s] 196 checks: 195 pass, 1 fail, 0 warn, 56 skip

--- failure enum_struct_variant_changed_kind: An enum struct variant changed kind ---

Description:
A pub enum's struct variant with at least one pub field has changed to a different kind of enum variant, breaking access to its pub fields.
      ref: https://doc.rust-lang.org/reference/items/enumerations.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/enum_struct_variant_changed_kind.ron

Failed in:
variant SumType::General in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/types.rs:180

   Summary semver requires new major version: 1 major and 0 minor checks failed
  Finished [  51.390s] hugr-core
  Building hugr-llvm v0.27.1 (current)
     Built [  29.446s] (current)
   Parsing hugr-llvm v0.27.1 (current)
    Parsed [   0.012s] (current)
  Building hugr-llvm v0.27.1 (baseline)
     Built [  29.563s] (baseline)
   Parsing hugr-llvm v0.27.1 (baseline)
    Parsed [   0.012s] (baseline)
  Checking hugr-llvm v0.27.1 -> v0.27.1 (assume minor change)
   Checked [   0.039s] 196 checks: 196 pass, 56 skip
   Summary no semver update required
  Finished [  60.094s] hugr-llvm
  Building hugr-model v0.27.1 (current)
     Built [  10.715s] (current)
   Parsing hugr-model v0.27.1 (current)
    Parsed [   0.017s] (current)
  Building hugr-model v0.27.1 (baseline)
     Built [  10.711s] (baseline)
   Parsing hugr-model v0.27.1 (baseline)
    Parsed [   0.016s] (baseline)
  Checking hugr-model v0.27.1 -> v0.27.1 (assume minor change)
   Checked [   0.029s] 196 checks: 196 pass, 56 skip
   Summary no semver update required
  Finished [  22.045s] hugr-model
  Building hugr-persistent v0.6.1 (current)
     Built [  22.201s] (current)
   Parsing hugr-persistent v0.6.1 (current)
    Parsed [   0.008s] (current)
  Building hugr-persistent v0.6.1 (baseline)
     Built [  23.346s] (baseline)
   Parsing hugr-persistent v0.6.1 (baseline)
    Parsed [   0.007s] (baseline)
  Checking hugr-persistent v0.6.1 -> v0.6.1 (assume minor change)
   Checked [   0.012s] 196 checks: 196 pass, 56 skip
   Summary no semver update required
  Finished [  46.330s] hugr-persistent

@acl-cqc acl-cqc added this to the hugr-rs 0.28.0 milestone Apr 10, 2026
@acl-cqc acl-cqc force-pushed the acl/boundless_type branch from 4ceb59e to 2dec804 Compare April 28, 2026 10:32
Base automatically changed from acl/type_wraps_term to main May 5, 2026 12:24
@acl-cqc acl-cqc force-pushed the acl/boundless_type branch from 2dec804 to b45e61f Compare May 5, 2026 12:48
Comment thread hugr-core/src/types.rs
&self.rows
}

pub(crate) fn rows_mut(&mut self) -> &mut [TypeRowRV] {
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.

Note this allows bypassing the update of the bound, so we keep it crate-private (and use it only for things where we are sure the bound doesn't change - namely, extension resolution)

Comment thread hugr-core/src/types.rs
/// [TypeDef]: crate::extension::TypeDef
pub(crate) fn validate(&self, var_decls: &[TypeParam]) -> Result<(), SignatureError> {
self.0.validate(var_decls)?;
// ALAN even this should be only a debug-assert really:
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.

This temporary debris from #2895 has now moved to a proper home in Term::validate

@acl-cqc acl-cqc marked this pull request as ready for review May 5, 2026 13:20
@acl-cqc acl-cqc requested a review from a team as a code owner May 5, 2026 13:20
@acl-cqc acl-cqc requested a review from doug-q May 5, 2026 13:20
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.

2 participants