Skip to content

Conversation

@lmondada
Copy link
Contributor

@lmondada lmondada commented Sep 30, 2025

This PR improves persistent-hugr performance. It does three things

  1. Introduces PersistentHugrCache to cache the list of children of each commit.
  2. Improves the implementation of HugrView::children by adding a custom iterator, instead of materialising the entire hugr in memory.
  3. small refactor: move fn node_status and enum NodeStatus from hugr-persistent/src/wire.rs to hugr-persistent/src/persistent_hugr.rs

Note:

Beyond the first two children, the order of

persistent_hugr.children(node)

and

let hugr = persistent_hugr.to_hugr();
hugr.children(node);

may no longer match when node is a Dataflow parent. This is annoying but I could not find an easy way to fix this. AFAIK this should not lead to observable differences according to current specs, but might be an issue at some point. I've created issue #2618 to track this.

@codecov
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

❌ Patch coverage is 95.37037% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.39%. Comparing base (3e12b65) to head (b9f183a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hugr-persistent/src/trait_impls/utils.rs 93.33% 2 Missing ⚠️
hugr-persistent/src/persistent_hugr.rs 95.65% 0 Missing and 1 partial ⚠️
hugr-persistent/src/trait_impls.rs 97.72% 0 Missing and 1 partial ⚠️
hugr-persistent/src/wire.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2595      +/-   ##
==========================================
+ Coverage   83.36%   83.39%   +0.02%     
==========================================
  Files         257      259       +2     
  Lines       50664    50741      +77     
  Branches    46187    46264      +77     
==========================================
+ Hits        42238    42313      +75     
- Misses       6059     6061       +2     
  Partials     2367     2367              
Flag Coverage Δ
python 91.53% <ø> (ø)
rust 82.60% <95.37%> (+0.02%) ⬆️

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.

Base automatically changed from lm/phugr-relrc5 to main October 12, 2025 01:41
@lmondada lmondada force-pushed the lm/phugr-caching branch 2 times, most recently from cb8268a to 357d42a Compare October 12, 2025 02:22
@lmondada lmondada marked this pull request as ready for review October 12, 2025 02:56
@lmondada lmondada requested a review from a team as a code owner October 12, 2025 02:56
@lmondada lmondada requested a review from aborgna-q October 12, 2025 02:56
Copy link
Collaborator

@aborgna-q aborgna-q left a comment

Choose a reason for hiding this comment

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

Nice!

It's a bit annoying that we need to force evaluation of the whole children when popping stuff from the back, but there's no way around it.

children_cache: HashMap<CommitId, Vec<CommitId>>,
}

impl PersistentHugrCache {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some docs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup will do!

Comment on lines 241 to 246
Either::Left(IterValidNodes::new(self, children.fuse()))
} else {
// children are precisely children of the commit hugr
Either::Right(children)
};
DoubleEndedIteratorAdapter::from(it)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need to wrap the IterValidNodes case, Either::Right(children) is already a doubleEndedIterator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, fixing that!

@lmondada
Copy link
Contributor Author

It's a bit annoying that we need to force evaluation of the whole children when popping stuff from the back, but there's no way around it.

True, but tbh it's barely (ever?) used backwards.

@lmondada lmondada enabled auto-merge October 13, 2025 12:59
@lmondada lmondada added this pull request to the merge queue Oct 13, 2025
Merged via the queue into main with commit 1d6c998 Oct 13, 2025
26 checks passed
@lmondada lmondada deleted the lm/phugr-caching branch October 13, 2025 13:12
@hugrbot hugrbot mentioned this pull request Oct 14, 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.

2 participants