clojure-rs / ClojureRS

Clojure, implemented atop Rust (unofficial)
Apache License 2.0
952 stars 27 forks source link

Refactor `invoke` to use Vec<Rc<Value>> #32

Closed Tko1 closed 4 years ago

Tko1 commented 4 years ago

Early on I decided to try a few different signatures for invoke to gain an intuition on what would work best.

Overall, I see that the program currently basically lives in a Rc<Value>, spending its time being 'seen' by many, and sometimes being cloned and transformed into a new value. It seems better to fit with the rest of the program and also just expect Rc<Value>s in invoke (ie, to go from

    fn invoke(&self,args: Vec<&Value>) -> Value {

to

     fn invoke(&self,args: Vec<Rc<Value>>) -> Value {

. Plus, I am currently in a situation where invoke needs a Rc<Value> of a &Value being passed in for one of the functions I'm implementing (assoc), and so to (currently) get this we basically have to needlessly clone &Value and Rc::new(..) that, causing the same Value to exist in multiple, independent spots in memory with their own family of Rc references. The nice thing is we are dealing with Values, not mutating entities with identity, so we should be able to efficiently store them once and reference them all over, rather than cloning them for each 'user' who wants to deal with one. I don't know if there are any unforseen trade offs of this

There are other places with indiscriminant cloning that likely can be changed a bit, but we will get to that

Off the top of my head, areas affected:

scrabsha commented 4 years ago

This would reduce the number of clones which are performed on Values. Do you think it would be possible to go further and eliminate every "clone on Value" in the codebase?

This would allow us to gain in execution speed, and we would be able to drop dyn-clone.