-
Notifications
You must be signed in to change notification settings - Fork 0
workflows: use TinySet/TinyMap in the workflow engine #269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 improves performance by replacing BTreeSet and BTreeMap with custom TinySet and TinyMap implementations optimized for small collections in the workflow engine. The new data structures use Vec-based linear scanning which is more efficient for small sizes (0-5 items) that are common in this context. Benchmarks show significant performance improvements with 8-21% reductions in instructions and cache hits.
- Introduces new TinySet/TinyMap data structures with Vec backing for small collections
- Replaces BTreeSet/BTreeMap usage throughout the workflow engine and related components
- Updates test code to use the new collection types
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
bd-log-primitives/src/tiny_set.rs | New TinySet and TinyMap implementations with Vec backing |
bd-workflows/src/workflow.rs | Replace BTreeMap with TinyMap for logs_to_inject and extraction fields |
bd-workflows/src/engine.rs | Update engine to use TinySet/TinyMap for buffer IDs and actions |
bd-workflows/src/actions_flush_buffers.rs | Replace BTreeSet with TinySet for buffer action management |
bd-workflows/src/workflow_test.rs | Update test expectations to use TinyMap |
profiling/src/main.rs | Use TinySet for workflow engine configuration |
Comments suppressed due to low confidence (1)
bd-workflows/src/workflow.rs:1
- The method call changed from
append
toextend
which has different semantics.append
drains the source collection whileextend
borrows it. This could cause performance differences or compilation errors if the source is expected to be consumed.
// shared-core - bitdrift's common client/server libraries
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
The observation is that we are using a large amount of BTreeSet and BTreeMap which often contains 0 entries, sometimes 1, and very rarely a few. The cost empty construction/destruction of these adds up. This introduces a Set/Map based purely on a Vec and linear scanning. main::benches::bench_workflow simple:| setup : Setup | setup.simple_workflow() Instructions: 1728|2197 (-21.3473%) [-1.27141x] L1 Hits: 2642|3443 (-23.2646%) [-1.30318x] LL Hits: 0|0 (No change) RAM Hits: 285|345 (-17.3913%) [-1.21053x] Total read+write: 2927|3788 (-22.7297%) [-1.29416x] Estimated Cycles: 12617|15518 (-18.6944%) [-1.22993x] main::benches::bench_workflow many_simple:| setup : Setup | setup.many_simple_workflows() Instructions: 44208|48512 (-8.87203%) [-1.09736x] L1 Hits: 70228|78101 (-10.0805%) [-1.11211x] LL Hits: 7|10 (-30.0000%) [-1.42857x] RAM Hits: 837|902 (-7.20621%) [-1.07766x] Total read+write: 71072|79013 (-10.0502%) [-1.11173x] Estimated Cycles: 99558|109721 (-9.26258%) [-1.10208x] Signed-off-by: Matt Klein <mklein@bitdrift.io>
The observation is that we are using a large amount of BTreeSet and BTreeMap which often contains 0 entries, sometimes 1, and very rarely a few. The cost empty construction/destruction of these adds up. This introduces a Set/Map based purely on a Vec and linear scanning.