Address feedback from PR #12229 #12242
Conversation
@sholderbach left a very helpful review and this just implements the suggestions he made. Didn't notice any difference in performance, but there could potentially be for a long running Nushell session or one that loads a lot of stuff.
| @@ -967,7 +966,7 @@ impl<'a> StateWorkingSet<'a> { | |||
| } | |||
| } | |||
|
|
|||
| for block in &self.permanent_state.blocks { | |||
| for block in self.permanent_state.blocks.iter() { | |||
There was a problem hiding this comment.
Sorry I'm not really sure, why are you using iter() here, but removing iter() call on crates/nu-cli/src/completions/completer.rs:L127?
There was a problem hiding this comment.
I'm not sure either! It probably just got caught up in some kind of intermediate change and I meant to change it back.
There was a problem hiding this comment.
Thanks for continuing to work on that!
Don't know if we have diverse enough benchmarks to see if there is a tradeoff for moving the reallocation burden from the clone to the first mutation of the Arc<Vec<T>>s (after the first make_mut it should remain pretty local).
Our looping benchmarks will clone the &EngineState a bunch of times but spend less time modifying and then merging StateDelta
| let block = Arc::make_mut(&mut block); | ||
| block.pipelines.drain(..block.pipelines.len() - 1); |
There was a problem hiding this comment.
Mhh not sure if this really makes it clearer that the block: &mut Arc<Block> in the outside scope now points to that mutated Arc inner. Which is relevant for the eval_block at the end of the function.
There was a problem hiding this comment.
I went back to the original order and added a comment.
| if !delta.blocks.is_empty() { | ||
| Arc::make_mut(&mut self.blocks).extend(delta.blocks); | ||
| } |
There was a problem hiding this comment.
Good call to elide that inner clone!
| @@ -2,7 +2,6 @@ use super::{usage::Usage, Command, EngineState, OverlayFrame, ScopeFrame, Variab | |||
| use crate::ast::Block; | |||
| use crate::Module; | |||
|
|
|||
| #[cfg(feature = "plugin")] | |||
There was a problem hiding this comment.
Oh, that went under the radar...
mental note: --no-default-features for the often dreamt of nightly extended test suite.
`let block` here is probably more confusing which value gets assigned the new `Arc`
Description
@sholderbach left a very helpful review and this just implements the suggestions he made.
Didn't notice any difference in performance, but there could potentially be for a long running Nushell session or one that loads a lot of stuff.
I also caught a bug where nu-protocol won't build without
pluginbecause of the previous conditional import. Oops. Fixed.User-Facing Changes
blocksandmodulestype inEngineStatechanged again. Shouldn't affect plugins or anything else though reallyTests + Formatting
toolkit fmttoolkit clippytoolkit testtoolkit test stdlibAfter Submitting