Refactor how we run FoldConstants
#30622
Conversation
ggevay
added
A-optimization
ggevay
force-pushed
the
fold-constants-fixpoint
branch
from
1b7b051
to
410c378
Compare
src/transform/src/lib.rs
Outdated
| // We need this to ensure that `CollectIndexRequests` gets a normalized plan. | ||
| // (For example, `FoldConstants` can break the normalized form by removing all | ||
| // references to a Let, see https://github.com/MaterializeInc/database-issues/issues/6371) | ||
| Box::new(normalize_lets::NormalizeLets::new(false)), | ||
| Box::new(typecheck::Typecheck::new(ctx.typecheck()).disallow_new_globals()), | ||
| Box::new(NormalizeLets::new(false)), |
There was a problem hiding this comment.
Very minor, but I think this NormalizeLets is now rendundant, because fold_constants_fixpoint() just above ended with it. That might be great news, in that it means we are a bit closer to having some expectations about the resulting state (the closer we get to ending with something like "canonicalization" the better, imo).
There was a problem hiding this comment.
Right, I've deleted the call now!
src/transform/src/lib.rs
Outdated
| // We might have arrived at a constant, e.g., due to contradicting literal constraints. | ||
| Box::new(fold_constants::FoldConstants { | ||
| Box::new(FoldConstants { |
There was a problem hiding this comment.
Do we not want the fixed point here?
There was a problem hiding this comment.
There are no Lets in the fast_path_optimizer, because this only runs on fast path peeks (as determined by create_fast_path_plan).
But we might need to alternate between ReduceScalars and FoldConstants, so now I've added a custom fixpoint loop that does just these two.
There was a problem hiding this comment.
Cool! What do you think about a future where normalize_lets is documented not to introduce lets, nor change the structure of let-free expressions (I think this is true / intended), at which point we could just use the general version. It would trade away "performance" but in the interest of "fewer special cases".
There was a problem hiding this comment.
Well, it seems ok to me that we have some special things in the fast_path_optimizer. If we were calling more general functions there, then I'd be a little bit afraid of possibly introducing optimizer performance regressions in the fast_path_optimizer when modifying those functions.
| // TODO: One could imagine CSEing multiple occurrences of a global Get | ||
| // to make us read from Persist only once. | ||
| // See <https://github.com/MaterializeInc/database-issues/issues/6363> |
There was a problem hiding this comment.
Was this comment ever right? I thought we dedup reads from persist at the source importing moment. Was it removed because it wasn't right? Or some other reason?
There was a problem hiding this comment.
It was never right!
I thought we dedup reads from persist at the source importing moment.
That's correct!
There was a problem hiding this comment.
Looks good to me. Some comments about unresolved questions, but they seem easy to resolve.
Fwiw, I don't think it is that hard to do constant inlining around Let and LetRec. If the value is constant, then (barring size concerns) inline it for every Get of a Let, or of a LetRec after the binding is formed (so that the collection is constant, rather than empty and then constant).
ggevay
force-pushed
the
fold-constants-fixpoint
branch
from
410c378
to
66096e1
Compare
I've created a follow-up issue for this: |
This PR refactors how we run
FoldConstantsin the first commit, and then does some minor cleanups in subsequent commits. (It's best to review it commit-by-commit.)As discussed in https://github.com/MaterializeInc/database-issues/issues/5346, the problem with the current way we call
FoldConstantsis thatFoldConstantscan't inline Lets, which can instead be done byNormalizeLets. Therefore, this PR's first commit creates a function that bundles togetherFoldConstantswithNormalizeLetsinto a fixpoint loop, so they can alternate when needed, and thus run the constant folding to completion. This new function is now called every time where the old code used to just callFoldConstants.Additionally, the PR also adds
ReduceScalarsin the same fixpoint loop, because that const-folds scalar expressions. (There was always a call toReduceScalarsimmediately afterFoldConstants, so this part is just a refactoring, where we make this "official".)There is only a trivial slt change, but in the past I've seen chaos in optimizer traces several times, where constant folding happened in a haphazard way rather then running to completion at the first call. So, it was just a matter of luck that things somehow always worked out fine at the end even with the old code in all our slts.
Note that
fold_constants_fixpoint()can be quadratic in the number of Lets in the worst case, but I think this is a smaller problem than things not getting const-folded all the way. (In the long term, we could consider changingFoldConstantsitself, so that it can do Let inlining on the fly, and then we wouldn't need to alternate withNormalizeLets. But this is not a trivial thing, as evidenced by the complexity of the inlining code inNormalizeLets.)Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.