Group expressions by first output#63
Conversation
| find_binary_expressions_left(&mut cn, cache, hashset_cache, n, k, r); | ||
| cn | ||
| }) | ||
| cache[k].exprs.par_iter().fold( |
There was a problem hiding this comment.
does changing this to fold make it faster? I doubt it though, because this is creating an extra temporary vec
There was a problem hiding this comment.
Generated by AI, it was slightly faster, so I have kept it. Unrelated to the pull request, too.
Pysearch performance is sensitive to slight changes in code. I will check again.
| let mut cn = CacheLevel::new(); | ||
| find_parens_expressions(&mut cn, cache, hashset_cache, n); | ||
| cn | ||
| (if n >= 4 && !is_leaf_expr(OP_INDEX_PARENS, n) { |
There was a problem hiding this comment.
this is just inlining a parallel find_parens_expressions? I don't think it's worth parallelizing because each task would only be doing one expr, so the parallel overhead is probably significant compared to the actual work, unless this will ends up doing deep dfs.
Also unrelated to the PR
There was a problem hiding this comment.
Similar to the previous. I will take a look.
| } | ||
| } | ||
| if BI_DIRECTIONAL { | ||
| if Matcher::MATCH_1BY1 && is_leaf_expr(op_idx, n) { |
There was a problem hiding this comment.
can we avoid duplicating this code
| .zip(INPUTS.iter()) | ||
| .all(|(&c, i)| c >= i.min_uses); | ||
| fn save(level: &mut Vec<Expr>, expr: Expr, n: usize, cache: &Cache, hashset_cache: &HashSetCache) { | ||
| if !EARLY_FIRST_ELEMENT_MATCH || Matcher::new().match_one(0, expr.output[0]) { |
There was a problem hiding this comment.
this will check first output twice, just to delay checking uses_required_vars, which is a cheap check anyway
| for e2 in &cache[n - k - op_len] { | ||
| find_binary_operators(cn, cache, hashset_cache, n, e1, e2, op_len); | ||
| find_binary_operators(cn, cache, hashset_cache, n, e2, e1, op_len); | ||
| if n > k + op_len { |
There was a problem hiding this comment.
can you revert back to the early return version?
if n <= k + op_len {
return;
}
once n <= k + op_len is true, it will be true for larger op_len as well
|
|
||
| let mut valid_ops = 0u32; | ||
|
|
||
| seq!(idx in 0..32 { |
There was a problem hiding this comment.
So we only want to support maximum of 32 binary ops? That seems fine to me, but I can imagine someone defining a bunch of custom ops, I think we should have a check somewhere if more than 32 are defined, instead of silently ignoring ops
| Some(o) => matcher.match_one(i, o), | ||
| None => false, | ||
| }) && matcher.match_final(Some(el), er, op_idx) { | ||
| println!("{el}{op_idx}{er}"); |
|
|
||
| /// If set to true, optimizes the search by grouping cached expressions by their first output value (`output[0]`). | ||
| /// This heuristic works better when `output[0]` probability of matching is low. | ||
| pub const EARLY_FIRST_ELEMENT_MATCH: bool = true; |
There was a problem hiding this comment.
This is related to matcher, so should be part of the Match trait like MATCH_1BY1, which is nicer because in there we can set a default value, so adding a new bool doesn't break any existing params
| } | ||
|
|
||
| #[inline(always)] | ||
| fn find_binary_operators_chunked_left<const OP_LEN: usize>( |
There was a problem hiding this comment.
should this just be find_binary_operators_chunked with BI_DIRECTIONAL false
There was a problem hiding this comment.
Yeah, tried that; it was slightly slower. That's why I reverted, but I will try again.
Edit: Yeah, made it use BI_DIRECTIONAL
|
I will create another pull request |
3 participants
x2 speedup <: