Eliminate a bunch of recursion #567
Conversation
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
from
311135f to
6250791
Compare
Introduces the new `iter` module which provides iteration abilities over various tree-like structures. This is largely copied from rust-simplicity, but is both simpler (because there is no sharing) and more complicated (because we need to handle n-ary nodes, not just binary ones). This currently uses Arc<[T]> in a number of places; we should eventually replace these with some sort of ArrayVec-type structure.
This is a breaking change because we no longer implement `ForEachKey` on `Terminal`. But IMHO we should never have implemented that trait (or any trait, really..) directly on `Terminal`. The fact that we did was just an implementation detail of how we used to do a lot of iteration.
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
3 times, most recently
from
654e234 to
0576007
Compare
| _ => {} | ||
| } | ||
| } | ||
| true |
There was a problem hiding this comment.
After reading about possible compiler optimization speedup from using functional style code. I feel we can write this as:
self.pre_order_iter().all(|ms| match ms.node {
Terminal::PkK(ref p) => pred(p),
Terminal::PkH(ref p) => pred(p),
Terminal::Multi(_, ref keys) | Terminal::MultiA(_, ref keys) => {
keys.iter().all(|k| pred(k))
}
_ => true,
})There was a problem hiding this comment.
|
Agreed, this is a step in the right direction. |
|
I can merge these and address the nits in a follow up PR. Just waiting for your response about the empty commits. |
|
@sanket1729 no, I'll remove the empty commits. I believe those occur during rebasing when my gpg agent fails to sign a commit. I'll do that and then you can merge this. For optimizations I think we should first introduce a wider set of benchmarks (miniscript parsing from string, decoding from script, serializing to string, encoding to script, then some borderline-trivial key conversions etc). |
This is again a breaking change because we remove the trait impl from `Terminal`, but again I don't think the trait impl should've existed. This also exposed an "off-label" use of `real_translate_pk` to convert context types as well as pk types, which I apparently explicitly reviewed and ACKed when reviewing rust-bitcoin#426, but which in retrospect looks a little funny. Rename this function to translate_pk_ctx so it's clear that it's doing two jobs.
As usual, we remove a method of the same name from `Terminal`, which is a breaking change.
As always, dropping the same method from Terminal
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
from
0576007 to
67e91b9
Compare
| @@ -0,0 +1,93 @@ | |||
| // Written in 2023 by Andrew Poelstra <[email protected]> | |||
There was a problem hiding this comment.
Hey @apoelstra, I saw this line and wondered if you added it as a habit or if I have been annoying you by removing them? Or was this PR just before we discussed all that? Cheers.
There was a problem hiding this comment.
I think it predates your PR to remove these. I would've just mildlessly copied the line from other files.
I'm certainly not annoyed :).
This introduces a new
itermodule with facilities for iterating over tree structures, and uses it to eliminate a bunch of recursion.There's a lot more "obvious" work after this, so I figured I'd PR this now for review and then do followup ones if it's accepted.
Then there's some less obvious work around getting rid of the recursion in
Expression::from_strand tightening up the memory allocation/locality there, and some even less obvious work in improving memory allocation/locality inMiniscript. But even if those later stages don't work out, this is a step in the right direction because it eliminates a bunch of stack overflow vectors, and reduces the amount of code.