Do not take ExprCompilerValue by value in as_compiled

Summary:
Change:

```
fn as_compiled(self) -> ExprCompiledValue { ... }
```

to

```
fn as_compiled(&self) -> ExprCompiledValue { ... }
```

We need to preserve original `ExprCompiledValue` after compilation to do the compilation again on freeze.

Unfortunately, this diff results in many string duplications at compile time. For example, in "get local" expression we need string to use it in error message. Now we clone the string.

This can be addressed for example by
* placing all strings into frozen heap (less memory safe)
* placing string in `Arc` (almost as expensive as string copy)
* creating some string table per module and store indices to strings (too complicated)

It string copy is the issue, perhaps the best thing to do is to stop allocating strings at parse time: we already have spans for the strings, so we can store strings as spans through the pipeline.

Reviewed By: ndmitchell

Differential Revision: D31034896

fbshipit-source-id: 8ea4776f3f8493cc6bd72ef27be8e3f564491dde

starlark/src/codemap.rs

@@ -121,6 +121,13 @@ impl<T> Spanned<T> {
             span: self.span,
         }
     }
+
+    pub fn map<U>(&self, f: impl FnOnce(&T) -> U) -> Spanned<U> {
+        Spanned {
+            node: f(&self.node),
+            span: self.span,
+        }
+    }
 }
 
 impl<T> Deref for Spanned<T> {

starlark/src/eval/fragment/call.rs

@@ -29,13 +29,14 @@ use crate::{
         fragment::expr::{get_attr_hashed, ExprCompiled, ExprCompiledValue, MaybeNot},
         Arguments, Evaluator, FrozenDef,
     },
+    gazebo::prelude::SliceExt,
     syntax::ast::{ArgumentP, ExprP},
     values::{
         function::NativeFunction, AttrType, FrozenStringValue, FrozenValue, StarlarkValue, Value,
         ValueLike,
     },
 };
-use gazebo::{coerce::coerce_ref, prelude::*};
+use gazebo::coerce::coerce_ref;
 use std::mem::MaybeUninit;
 
 #[derive(Default)]
@@ -69,10 +70,10 @@ pub(crate) enum CallCompiled {
 }
 
 impl Spanned<CallCompiled> {
-    pub(crate) fn as_compiled(self) -> ExprCompiled {
+    pub(crate) fn as_compiled(&self) -> ExprCompiled {
         let span = self.span;
         match self.node {
-            CallCompiled::Call(box (fun, args)) => {
+            CallCompiled::Call(box (ref fun, ref args)) => {
                 args!(
                     args,
                     expr!("call", fun, |eval| {
@@ -82,7 +83,7 @@ impl Spanned<CallCompiled> {
                     })
                 )
             }
-            CallCompiled::Frozen(box (this, fun, args)) => {
+            CallCompiled::Frozen(box (this, fun, ref args)) => {
                 if let Some(fun_ref) = fun.downcast_frozen_ref::<FrozenDef>() {
                     assert!(this.is_none());
                     args!(
@@ -121,7 +122,8 @@ impl Spanned<CallCompiled> {
                     )
                 }
             }
-            CallCompiled::Method(box (this, s, args)) => {
+            CallCompiled::Method(box (ref this, ref s, ref args)) => {
+                let s = s.clone();
                 args!(
                     args,
                     expr!("call_method", this, |eval| {
@@ -160,27 +162,26 @@ enum ArgsCompiledSpec {
 }
 
 impl ArgsCompiledValue {
-    fn spec(mut self) -> ArgsCompiledSpec {
+    fn spec(&self) -> ArgsCompiledSpec {
         if self.names.is_empty()
             && self.args.is_none()
             && self.kwargs.is_none()
             && self.pos_named.len() <= 2
         {
-            match self.pos_named.pop() {
-                None => ArgsCompiledSpec::Args0(Args0Compiled),
-                Some(a1) => match self.pos_named.pop() {
-                    None => ArgsCompiledSpec::Args1(Args1Compiled(a1.as_compiled())),
-                    Some(a2) => {
-                        ArgsCompiledSpec::Args2(Args2Compiled(a2.as_compiled(), a1.as_compiled()))
-                    }
-                },
+            match self.pos_named.as_slice() {
+                [] => ArgsCompiledSpec::Args0(Args0Compiled),
+                [a0] => ArgsCompiledSpec::Args1(Args1Compiled(a0.as_compiled())),
+                [a0, a1] => {
+                    ArgsCompiledSpec::Args2(Args2Compiled(a0.as_compiled(), a1.as_compiled()))
+                }
+                _ => unreachable!(),
             }
         } else {
             ArgsCompiledSpec::Args(ArgsCompiled {
-                pos_named: self.pos_named.into_map(|a| a.as_compiled()),
-                names: self.names,
-                args: self.args.map(|a| a.as_compiled()),
-                kwargs: self.kwargs.map(|a| a.as_compiled()),
+                pos_named: self.pos_named.map(|a| a.as_compiled()),
+                names: self.names.clone(),
+                args: self.args.as_ref().map(|a| a.as_compiled()),
+                kwargs: self.kwargs.as_ref().map(|a| a.as_compiled()),
             })
         }
     }

starlark/src/eval/fragment/compr.rs

@@ -126,15 +126,15 @@ fn compile_clauses(
     }
 }
 
-fn eval_list(x: Spanned<ExprCompiledValue>, mut clauses: Vec<ClauseCompiled>) -> ExprCompiled {
+fn eval_list(x: &Spanned<ExprCompiledValue>, clauses: &[ClauseCompiled]) -> ExprCompiled {
     if clauses.len() == 1 && clauses[0].ifs.is_empty() {
-        let c = clauses.pop().unwrap();
+        let c = clauses.last().unwrap();
         let ClauseCompiled {
-            var,
-            over,
+            ref var,
+            ref over,
             over_span,
-            ifs,
-        } = c;
+            ref ifs,
+        } = *c;
         assert!(ifs.is_empty());
         let x = x.as_compiled();
         let var = var.as_compiled();
@@ -168,9 +168,9 @@ fn eval_list(x: Spanned<ExprCompiledValue>, mut clauses: Vec<ClauseCompiled>) ->
 }
 
 fn eval_dict(
-    k: Spanned<ExprCompiledValue>,
-    v: Spanned<ExprCompiledValue>,
-    clauses: Vec<ClauseCompiled>,
+    k: &Spanned<ExprCompiledValue>,
+    v: &Spanned<ExprCompiledValue>,
+    clauses: &[ClauseCompiled],
 ) -> ExprCompiled {
     let k_span = k.span;
     let k = k.as_compiled();
@@ -198,10 +198,10 @@ pub(crate) enum ComprCompiled {
 }
 
 impl ComprCompiled {
-    pub(crate) fn as_compiled(self) -> ExprCompiled {
-        match self {
-            ComprCompiled::List(box x, clauses) => eval_list(x, clauses),
-            ComprCompiled::Dict(box (k, v), clauses) => eval_dict(k, v, clauses),
+    pub(crate) fn as_compiled(&self) -> ExprCompiled {
+        match *self {
+            ComprCompiled::List(box ref x, ref clauses) => eval_list(x, clauses),
+            ComprCompiled::Dict(box (ref k, ref v), ref clauses) => eval_dict(k, v, clauses),
         }
     }
 }
@@ -217,7 +217,7 @@ pub(crate) struct ClauseCompiled {
 // lifetimes to express it :(
 
 fn eval_one_dimensional_comprehension_dict(
-    mut clauses: Vec<ClauseCompiled>,
+    clauses: &[ClauseCompiled],
     add: Box<
         dyn for<'v> Fn(
                 &mut SmallMap<Value<'v>, Value<'v>>,
@@ -234,16 +234,16 @@ fn eval_one_dimensional_comprehension_dict(
         + Send
         + Sync,
 > {
-    if let Some(c) = clauses.pop() {
+    if let Some((c, clauses)) = clauses.split_last() {
         let ClauseCompiled {
-            var,
-            over,
+            ref var,
+            ref over,
             over_span,
-            ifs,
-        } = c;
+            ref ifs,
+        } = *c;
         let over = over.as_compiled();
         let var = var.as_compiled();
-        let ifs = ifs.into_map(|c| c.as_compiled());
+        let ifs = ifs.map(|c| c.as_compiled());
         let rest = eval_one_dimensional_comprehension_dict(clauses, add);
         box move |accumulator, eval| {
             // println!("eval1 {:?} {:?}", ***e, clauses);
@@ -265,7 +265,7 @@ fn eval_one_dimensional_comprehension_dict(
 }
 
 fn eval_one_dimensional_comprehension_list(
-    mut clauses: Vec<ClauseCompiled>,
+    clauses: &[ClauseCompiled],
     add: Box<
         dyn for<'v> Fn(&mut Vec<Value<'v>>, &mut Evaluator<'v, '_>) -> Result<(), ExprEvalException>
             + Send
@@ -276,16 +276,16 @@ fn eval_one_dimensional_comprehension_list(
         + Send
         + Sync,
 > {
-    if let Some(c) = clauses.pop() {
+    if let Some((c, clauses)) = clauses.split_last() {
         let ClauseCompiled {
-            var,
-            over,
+            ref var,
+            ref over,
             over_span,
-            ifs,
-        } = c;
+            ref ifs,
+        } = *c;
         let over = over.as_compiled();
         let var = var.as_compiled();
-        let ifs = ifs.into_map(|c| c.as_compiled());
+        let ifs = ifs.map(|c| c.as_compiled());
         let rest = eval_one_dimensional_comprehension_list(clauses, add);
         box move |accumulator, eval| {
             // println!("eval1 {:?} {:?}", ***e, clauses);

starlark/src/eval/fragment/def.rs

@@ -86,6 +86,7 @@ impl AtomicFrozenModuleOption {
     }
 }
 
+#[derive(Clone)]
 struct ParameterName {
     name: String,
     captured: Captured,
@@ -100,15 +101,19 @@ enum ParameterCompiled<T> {
 }
 
 impl<T> ParameterCompiled<T> {
-    fn map_expr<U>(self, f: impl Fn(T) -> U) -> ParameterCompiled<U> {
+    fn map_expr<U>(&self, f: impl Fn(&T) -> U) -> ParameterCompiled<U> {
         match self {
-            ParameterCompiled::Normal(n, o) => ParameterCompiled::Normal(n, o.map(f)),
+            ParameterCompiled::Normal(n, o) => {
+                ParameterCompiled::Normal(n.clone(), o.as_ref().map(f))
+            }
             ParameterCompiled::WithDefaultValue(n, o, t) => {
-                ParameterCompiled::WithDefaultValue(n, o.map(&f), f(t))
+                ParameterCompiled::WithDefaultValue(n.clone(), o.as_ref().map(&f), f(t))
             }
             ParameterCompiled::NoArgs => ParameterCompiled::NoArgs,
-            ParameterCompiled::Args(n, o) => ParameterCompiled::Args(n, o.map(f)),
-            ParameterCompiled::KwArgs(n, o) => ParameterCompiled::KwArgs(n, o.map(f)),
+            ParameterCompiled::Args(n, o) => ParameterCompiled::Args(n.clone(), o.as_ref().map(f)),
+            ParameterCompiled::KwArgs(n, o) => {
+                ParameterCompiled::KwArgs(n.clone(), o.as_ref().map(f))
+            }
         }
     }
 
@@ -174,15 +179,16 @@ pub(crate) struct DefCompiled {
 }
 
 impl DefCompiled {
-    pub(crate) fn as_compiled(self) -> ExprCompiled {
+    pub(crate) fn as_compiled(&self) -> ExprCompiled {
         let DefCompiled {
-            function_name,
-            params,
-            return_type,
+            ref function_name,
+            ref params,
+            ref return_type,
             info,
-        } = self;
-        let params = params.into_map(|p| p.into_map(|p| p.map_expr(|e| e.as_compiled())));
-        let return_type = return_type.map(|t| Spanned {
+        } = *self;
+        let function_name = function_name.clone();
+        let params = params.map(|p| p.map(|p| p.map_expr(|e| e.as_compiled())));
+        let return_type = return_type.as_ref().map(|t| Spanned {
             span: t.span,
             node: t.as_compiled(),
         });