Replace macros with traits; other cleanups #650
Conversation
These are fairly common leaf nodes and throughout the codebase we tend to reconstruct and typecheck them. It is faster and clearer to just have constants.
This eliminates a bunch of error paths, which is nice.
In a later commit we will rewrite Terminal::from_tree entirely. But the beginning and end (which parse out and then apply "wrappers", the modifiers added to miniscript fragments using `:`) will remain the same. Pull this logic out into a pair of helper functions. This also reduces the total amount of indendentation, reduces function size, and reduces the total number of variables in scope at once. So this is a useful refactor independent of any future work.
On my system these show (the last two lines are the new ones) test expression::benches::parse_tree ... bench: 860 ns/iter (+/- 44) test expression::benches::parse_tree_deep ... bench: 1,481 ns/iter (+/- 66) test miniscript::benches::parse_segwit0 ... bench: 9,205 ns/iter (+/- 77) test miniscript::benches::parse_segwit0_deep ... bench: 23,003 ns/iter (+/- 248) As mentioned in the last commit message, these numbers are 10-20x the numbers for parsing expressions, meaning that optimizing the expression parser by itself will not get us a large performance boost.
apoelstra
force-pushed
the
2024-03--cleanups
branch
from
f5c35c3 to
164bde9
Compare
|
Rebased. |
| impl_from_str!( | ||
| Bare<Pk>, | ||
| type Err = Error;, | ||
| impl<Pk: crate::FromStrKey> core::str::FromStr for Bare<Pk> { |
There was a problem hiding this comment.
nit: I rekon we should import FromStrKey and use the more terse <Pk: FromStrKey> since inline trait bounds are noisy enough as it is.
There was a problem hiding this comment.
Agreed. Will do this as a followup (or you are welcome to).
There was a problem hiding this comment.
ACK 164bde9. Are you taking a performance refactor stab at rust-miniscript?
Sorta -- my original goal was to test a more-efficient tree structure, starting with From there I've gotten a bit sidetracked improving the errors in this crate and improving other things (like these macros) which I ran into. My next PR will bring in a new |
There are several macros we use to implement functions when our
Pktypes satisfy a large bundle of trait conditions. There is a trick rust-lang/rust#20671 (comment) that we can use instead.While I'm at it, do several other small cleanups.
As a weekend project I rewrote the expression module and was able to get a 10-15% speedup on parsing miniscripts, while eliminating recursion and simplifying the algorithms. I am still working on cleaning up the code and improving the error handling. This is the first set of commits from that branch, which should be simple and uncontroversial.