[draft] [opinions welcome] Change the individual callback functions in run_program to a trait which simplifies lifetime management #398
Conversation
Pull Request Test Coverage Report for Build 8850932024Details
💛 - Coveralls |
|
thanks @Rigidity for the typedef suggestion and catching the ungated declaration. |
| &mut self, | ||
| _allocator: &mut Allocator, | ||
| _pass: PreEvalIndex, | ||
| _result: Option<NodePtr>, |
There was a problem hiding this comment.
I wonder why I did not just include the result verbatim, ie. use Result instead of Option. Maybe it was trying to be more like the original python? I wouldn't necessarily be opposed to changing this.
src/run_program.rs
Outdated
| fn post_eval( | ||
| &mut self, | ||
| _allocator: &mut Allocator, | ||
| _pass: PreEvalIndex, |
There was a problem hiding this comment.
Is this just a nonce to match with the PreEval sexp/args pair? I suppose _pass means something when this is used with the classic compiler, but I wonder if a more generic name might be better.
There was a problem hiding this comment.
Hmm ya, the name could improve. I was thinking 'passthrough' but will give it some thought.
| if let Some(pre_eval) = &mut self.pre_eval { | ||
| if let PreEvalResult::CallPostEval(pass) = | ||
| pre_eval.pre_eval(self.allocator, program, env)? | ||
| { |
There was a problem hiding this comment.
This suggests to me that you probably need to do a cargo fmt.
There was a problem hiding this comment.
PreEvalResult::CallPostEval is longer so it re-wraps a bit. This is post fmt.
There was a problem hiding this comment.
Yeah fmt can be a bit funky at times, but that's the price of consistency I suppose :P
There was a problem hiding this comment.
Interesting, I don't recall having seen a lone { like this before.
src/run_program.rs
Outdated
| Operation::Apply => { | ||
| let apply_op_res = self.apply_op(cost, effective_max_cost - cost); | ||
| augment_cost_errors(apply_op_res, max_cost_ptr)? | ||
| } |
There was a problem hiding this comment.
It seems like this little diff is just the exact same code in a different form, yes? I'm wondering if this change is intentional.
There was a problem hiding this comment.
ya changed that back.
| #[cfg(feature = "pre-eval")] | ||
| /// Implementing this trait allows an object to be notified of clvm operations | ||
| /// being performed as they happen. | ||
| pub trait PreEval { |
There was a problem hiding this comment.
I wonder if there's now a better name for this, since it could potentially implement both pre and post eval. Maybe Tracer or EvalLogger or InvocationTracker or CallMonitor or something. (Thanks to ChatGPT for some of these ideas.)
PreEval is maybe more useful for people who know the code already, but that's maybe eight out of eight billion. :)
There was a problem hiding this comment.
Besides some naming nits (and possibly turning an Option a Result as it seems like it may have been a mistake in the original API), it seems like a pretty reasonable way for this code to evolve. I think you (Arty) are the only one using this functionality these days, and it seems like a clear win.
Arvid already has gated PreEval behind a feature flag so we don't have to worry much about it affecting performance in production.
I didn't look closely at the tests, but I assume you just made whatever changes you needed to make the tests work.
|
I think performance should still be a consideration, since features are additive - if you depend on |
This makes PreEval a trait which is passed down the stack as a mutable reference. It contains both pre_eval and post_eval methods and calls post_eval when requested based on the result of pre_eval as before.
Because the callback's lifetime is now easily determined to be the lifetime of the run_program call and the other object that could be of use, the allocator is also passed by a mutable reference, the caller has maximal freedom in how their implementation of PreEval works without having to use the Rc<RefCell<...>> pattern and cunning use of move closures to create the callbacks and avoids construction of a Box on each callback for the second closure.
Since this changes the form of the run_program interface, commentary on that is welcome. This is a response to the arch meeting discussion rather than something I'm actively requesting, but I do think this is better overall.