wip: use arena-style cache when simplifying Expressions (DO NOT MERGE) #276
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
…ltered (#268) * wip: test that similar calibrations with different sites are not de-duplicated * use snapshot testing, rstest file globbing
genos
reviewed
Aug 9, 2023
| b.hash(state); | ||
| } | ||
| //InfixOperator::Plus | InfixOperator::Star => { | ||
| // // commutative, so put left & right in decreasing order by hash value |
There was a problem hiding this comment.
I agree that this is costly, but we put it in for #27. Are we deciding to no longer ensure that, e.g., 1 + x == x + 1?
There was a problem hiding this comment.
Alternatively, should we just derive Hash, PartialEq, and Eq and call it a day?
There was a problem hiding this comment.
In fact, I think this is the way to go. #27 only handles commutativity, but not other rules that lead to equality between expressions. Since we're simplifying expressions with these rules, enforcing some but not all in the hashing & equality implementations seems wrong and wasteful. I'll open a new PR.
genos
reviewed
Aug 9, 2023
| if let Ok(simpler) = simplification::run(self) { | ||
| *self = simpler; | ||
| } | ||
| let temp = std::mem::replace(self, Expression::PiConstant); |
There was a problem hiding this comment.
I really need to remember this trick 👍
genos
pushed a commit
that referenced
this pull request
Aug 10, 2023
BREAKING: Expressions no longer use hashing for implementing equality BREAKING: Expression equality no longer takes commutativity into account In #276, @Shadow53 noted > One thing that may be useful enough to pull into its own PR is the > change to not use hashing in the implementation of `PartialEq` for > `Expression`, which also helps speed things up. We originally put this together in #27 to ensure that equality held in the face of commutativity, e.g., `1 + x == x + 1`. In addition to the performance benefits of decoupling the hashing and equality implementations, it makes sense to remove any special status for commutative operations in light of all the work we're doing on expression simplification. If we wished to ensure expressions are `Eq` if and only if they represent the same mathematical expression, we'd have to have equality contingent upon simplification, which would be even more costly.
genos
added a commit
that referenced
this pull request
Aug 10, 2023
* feat!: Decouple expression hashing and equality BREAKING: Expressions no longer use hashing for implementing equality BREAKING: Expression equality no longer takes commutativity into account In #276, @Shadow53 noted > One thing that may be useful enough to pull into its own PR is the > change to not use hashing in the implementation of `PartialEq` for > `Expression`, which also helps speed things up. We originally put this together in #27 to ensure that equality held in the face of commutativity, e.g., `1 + x == x + 1`. In addition to the performance benefits of decoupling the hashing and equality implementations, it makes sense to remove any special status for commutative operations in light of all the work we're doing on expression simplification. If we wished to ensure expressions are `Eq` if and only if they represent the same mathematical expression, we'd have to have equality contingent upon simplification, which would be even more costly. * fix: Use Self::
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.
This PR is meant as a reference and not to be merged (yet).
It is based on @genos'
simplify-by-handbranch and attempts to improve performance by using an arena-based cache duringExpressionsimplification.Some internal benchmarks seem to show a good improvement (~17%) in runtime when the cache is actually useful, but an overhead of 1-5% when not useful (no repeated expressions). This may be conflated with the item below, which also helps performance -- I did not benchmark them separately.
One thing that may be useful enough to pull into its own PR is the change to not use hashing in the implementation of
PartialEqforExpression, which also helps speed things up.