Block machines: Skip complete identities #1528
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
georgwiese
commented
Jul 3, 2024
Comment on lines
-498
to
-535
| // First check if we already store the value. | ||
| // This can happen in the loop detection case, where this function is just called | ||
| // to validate the constraints. | ||
| if outer_query.left.iter().all(|v| v.is_constant()) { | ||
| // All values on the left hand side are known, check if this is a query | ||
| // to the last row. | ||
| if let Some(row_index) = self.last_latch_row_index() { | ||
| let current = &self.get_row(row_index); | ||
| let fresh_row = Row::fresh(self.fixed_data, row_index + 1); | ||
| let next = if self.latch_row == self.block_size - 1 { | ||
| // We don't have the next row, because it would be the first row of the next block. | ||
| // We'll use a fresh row instead. | ||
| &fresh_row | ||
| } else { | ||
| self.get_row(row_index + 1) | ||
| }; | ||
|
|
||
| let row_pair = RowPair::new( | ||
| current, | ||
| next, | ||
| row_index, | ||
| self.fixed_data, | ||
| UnknownStrategy::Unknown, | ||
| ); | ||
|
|
||
| let mut identity_processor = IdentityProcessor::new(self.fixed_data, mutable_state); | ||
| if let Ok(result) = identity_processor.process_link(&outer_query, &row_pair) { | ||
| if result.is_complete() && result.constraints.is_empty() { | ||
| log::trace!( | ||
| "End processing block machine '{}' (already solved)", | ||
| self.name() | ||
| ); | ||
| return Ok(result); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Finally, we can remove this trouble maker! It is very error prone and had to be adjusted many times in the past.
Commit 9b042b2 asserts that it would never reach the return with the new changes (you can see the CI passes on this commit).
Basically, this code checks if the LHS we are getting happens to be exactly what is in the latch row of the last block that was added. But that only happens in practice if the identity is processed even though it was completed already. The VmProcessor already makes sure not to do that, and with this PR the BlockProcessor does too.
chriseth
approved these changes
Jul 3, 2024
This was referenced Jul 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just like the
VmProcessor, with this PR theBlockProcessornever processes an identity again that has been completed.This should be a slight performance optimization (when the "default" sequence iterator is used), but more importantly it: