Conversation
apoelstra
force-pushed
the
2026-08/compiler-opts-1
branch
2 times, most recently
from
August 24, 2026 01:52
03b9b1e to
d4a97c6
Compare
Member
Author
|
On d4a97c6 successfully ran local tests |
Abeeujah
reviewed
Aug 31, 2026
Comment on lines
+136
to
+139
| dissat_cost: match (left.dissat_cost, right.dissat_cost) { | ||
| (Some(l), Some(r)) => Some(l + r), | ||
| _ => None, | ||
| }, |
Contributor
There was a problem hiding this comment.
This can leverage the zip and map methods on Option<T>
Suggested change
| dissat_cost: match (left.dissat_cost, right.dissat_cost) { | |
| (Some(l), Some(r)) => Some(l + r), | |
| _ => None, | |
| }, | |
| dissat_cost: left.dissat_cost.zip(right.dissat_cost).map(|(l, r)| l + r), |
Comment on lines
+259
to
+271
| dissat_cost: if let (Some(ldis), Some(rdis)) = (left.dissat_cost, right.dissat_cost) { | ||
| if (2.0 + ldis) > (1.0 + rdis) { | ||
| Some(1.0 + rdis) | ||
| } else { | ||
| Some(2.0 + ldis) | ||
| } | ||
| } else if let Some(ldis) = left.dissat_cost { | ||
| Some(2.0 + ldis) | ||
| } else if let Some(rdis) = right.dissat_cost { | ||
| Some(1.0 + rdis) | ||
| } else { | ||
| None | ||
| }, |
Contributor
There was a problem hiding this comment.
// Complex if/let is better as is.
Unsure what the argument is, but this whole if/let can be simplified using match
Suggested change
| dissat_cost: if let (Some(ldis), Some(rdis)) = (left.dissat_cost, right.dissat_cost) { | |
| if (2.0 + ldis) > (1.0 + rdis) { | |
| Some(1.0 + rdis) | |
| } else { | |
| Some(2.0 + ldis) | |
| } | |
| } else if let Some(ldis) = left.dissat_cost { | |
| Some(2.0 + ldis) | |
| } else if let Some(rdis) = right.dissat_cost { | |
| Some(1.0 + rdis) | |
| } else { | |
| None | |
| }, | |
| dissat_cost: match ( | |
| left.dissat_cost.map(|l| 2.0 + l), | |
| right.dissat_cost.map(|r| 1.0 + r), | |
| ) { | |
| (Some(l), Some(r)) => Some(l.min(r)), | |
| (left, right) => left.or(right), | |
| }, |
Member
Author
There was a problem hiding this comment.
Nice! Clippy wanted me to turn it into a flat series of } else if {s.
apoelstra
force-pushed
the
2026-08/compiler-opts-1
branch
from
August 31, 2026 21:12
d4a97c6 to
8fd590e
Compare
Member
Author
|
Addressed @Abeeujah's commenst. |
Member
Author
|
On 8fd590e successfully ran local tests |
Member
Author
|
8fd590e needs rebase |
It feels a little weird to export this at the root but there isn't anywhere else in the public API where it really fits, so just dump it there.
If we want to regression test the Concrete parser, that's fine but belongs in a separate fuzztest (which I may add in a separate PR). But for testing the compiler, we definitely want to be synthesizing policies directly rather than parsing and rejecting.
Gets rid of a couple 'as f64' casts.
These policies exercise paths that earlier iterations of this PR introduced bugs to. (The bugs were found by fuzzing.)
Better encapsulation, and I want to clean up these types. Code move only.
The Type::type_check function takes a terminal, matches on it, calls the appropriate method on Type, and wraps it in a nice error. It turns out that in the compiler, this is obscenely slow. I'm not sure if it's because the big match undermines inlining, or if the error-wrapping (which the compiler doesn't even use; it only uses typeck errors as a binary "ok or not?" signal), or what. But empirically, replacing the call to Type::type_check with direct calls to the methods on Type, results in a massive compiler speedup. At the cost of a bit more code repetition. IMO definitely worth it. You shouldn't trust these benchmarks too much because my system was busy, but the difference is quite extreme: Before: test benchmarks::compiler_benches::compile_basic ... bench: 6,285,527.70 ns/iter (+/- 4,895,149.56) test benchmarks::compiler_benches::compile_large ... bench: 14,602,446.20 ns/iter (+/- 9,250,348.77) test benchmarks::compiler_benches::compile_large_tap ... bench: 982,121,582.90 ns/iter (+/- 1,209,655,507.47) test benchmarks::compiler_benches::compile_xlarge ... bench: 582,951,636.20 ns/iter (+/- 58,752,609.07) After: test benchmarks::compiler_benches::compile_basic ... bench: 1,520,658.20 ns/iter (+/- 13,249.16) test benchmarks::compiler_benches::compile_large ... bench: 4,128,737.40 ns/iter (+/- 55,408.00) test benchmarks::compiler_benches::compile_large_tap ... bench: 716,569,601.10 ns/iter (+/- 130,612,914.15) test benchmarks::compiler_benches::compile_xlarge ... bench: 148,633,378.80 ns/iter (+/- 16,348,954.09) On `compile_large_tap` it's "only" a 35% speedup but on the others, and on the `segwit_limits` unit tests, it's a 3-4x speedup. Wild.
This is a weird structure. I'm not sure what I was thinking here. It's 4 function pointers in a struct, with a `cast()` method on them that calls all the functions in the right order to transform an AstElemExt. Why not just make it an AstElemExt method directly? I did this to simplify the code, and it also comes with another fairly big speedup (though this one is small enough that it's plausibly just noise from my busy machine). The weird order is copied from the old logic. I don't think there's any rhyme or reason to it, but if we change the order then some compilations can change (e.g. swapping the l and n wrappers, which functionally commute). Before (copied from the "after" from my previous commit) test benchmarks::compiler_benches::compile_basic ... bench: 1,520,658.20 ns/iter (+/- 13,249.16) test benchmarks::compiler_benches::compile_large ... bench: 4,128,737.40 ns/iter (+/- 55,408.00) test benchmarks::compiler_benches::compile_large_tap ... bench: 716,569,601.10 ns/iter (+/- 130,612,914.15) test benchmarks::compiler_benches::compile_xlarge ... bench: 148,633,378.80 ns/iter (+/- 16,348,954.09) After: test benchmarks::compiler_benches::compile_basic ... bench: 1,199,013.60 ns/iter (+/- 28,096.21) test benchmarks::compiler_benches::compile_large ... bench: 3,350,305.90 ns/iter (+/- 463,149.87) test benchmarks::compiler_benches::compile_large_tap ... bench: 703,996,003.60 ns/iter (+/- 835,362,175.58) test benchmarks::compiler_benches::compile_xlarge ... bench: 140,988,430.90 ns/iter (+/- 24,692,231.52) seems like 30% or so on the small ones, 5% or so on the big ones. Nice.
This simplifies and cleans up the code, but with a pretty severe performance hit for the "small" benchmarks, in exchange for a improvement on the larger ones. Will keep an eye on this. Before: test benchmarks::compiler_benches::compile_basic ... bench: 1,199,013.60 ns/iter (+/- 28,096.21) test benchmarks::compiler_benches::compile_large ... bench: 3,350,305.90 ns/iter (+/- 463,149.87) test benchmarks::compiler_benches::compile_large_tap ... bench: 703,996,003.60 ns/iter (+/- 835,362,175.58) test benchmarks::compiler_benches::compile_xlarge ... bench: 140,988,430.90 ns/iter (+/- 24,692,231.52) After: test benchmarks::compiler_benches::compile_basic ... bench: 1,570,666.35 ns/iter (+/- 1,066,467.89) test benchmarks::compiler_benches::compile_large ... bench: 6,010,996.75 ns/iter (+/- 3,744,165.23) test benchmarks::compiler_benches::compile_large_tap ... bench: 694,140,897.60 ns/iter (+/- 656,723,194.35) test benchmarks::compiler_benches::compile_xlarge ... bench: 135,546,606.00 ns/iter (+/- 11,686,439.12)
There is no longer any reason to keep this as a separate data structure.
apoelstra
force-pushed
the
2026-08/compiler-opts-1
branch
from
September 3, 2026 13:06
8fd590e to
482b0a1
Compare
Member
Author
|
On 482b0a1 successfully ran local tests |
Member
Author
|
482b0a1 needs rebase |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 is a followup to #988 and starts a project to optimize the compiler. It pulls apart the compiler module into multiple submodules, inlines the
CompilerExtDatastruct (which since #988 is just a pair of floats which never change after construction) (previously it also had abranch_probvalue that would be mutated during compilation).This also cleans up the cast-computation logic, type checking within the compiler, and eliminates a few other places where we were passing miscellaneous closures around. It leaves the codebase in a much cleaner state.
Some of the commits have benchmark data showing a significant speedup, but I don't have a spare computer right now to run clean before and after benchmarks on the whole branch. Will update the PR with that data once I have it.
It also greatly speeds up the existing regression test between the compiler on master and the one released on 12.x. I ran 150 MM iterations just now, plus a whole bunch when I was developing the branch some months ago.