- 
                Notifications
    
You must be signed in to change notification settings  - Fork 324
 
feat(query-planner,composition): Allow specifying a limit on the number of in-memory graph paths, weighted by path size #8488
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
base: dev
Are you sure you want to change the base?
Conversation
          ✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 0d87603aead88469ae50a455  | 
    
| 
           @sachindshinde, please consider creating a changeset entry in   | 
    
09382ed    to
    6149f96      
    Compare
  
    6149f96    to
    09958a0      
    Compare
  
    …ghted by path size, and allow specifying a limit
09958a0    to
    3fb0eb6      
    Compare
  
    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.
It looks great! I just have a few remarks, but those are not a blocker.
Besides, I wish we studied the co-releation between high water mark and the actual max memory used.
BTW, we don't report high water mark from composition. That is something we would like to do once we finish porting composition.
| // Add options as needed - for now keeping it minimal | ||
| /// Maximum allowable number of outstanding subgraph paths to validate during satisfiability. | ||
| pub(crate) max_validation_subgraph_paths: Option<usize>, | ||
| /// Maximum allowable number of in-memory paths (weighted by path size) during satisfiability. | 
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.
Maybe it's a good time to move CompositionOptions to composition/mod.rs?
| /// A counter on the total weight of graph paths in use by the current task (e.g. query planning | ||
| /// or composition). This is here to facilitate dropping and graph paths derived from this one. | ||
| #[serde(skip)] | ||
| weight_counter: Arc<GraphPathWeightCounter>, | 
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.
The weight counter system makes sense. It's quite similar to custom allocator with a total size metric.
Just a thought: I'm afraid it may be too easy to forget increasing this counter at all construction sites. Wouldn't it be a good idea to make this counter a wrapper type like Weighed<GraphPath<...>>? And it may be reusable, too.
And it's quite fun to continue this analogy...
struct Weighed: one item that has a weight and put on aScale.Arc<Scale>: the sum of all weights with a limit.
No description provided.