Remove Evaluator.module_variables

Summary:
# Explanations for the whole stack: D58068275 to this

`Evaluator.module_variables` contains captured frozen module. Which is used mostly to access frozen module variables from unfrozen functions (e.g. lambdas).

Before this diff, on each `def` enter we updated this variable.

This is rarely needed, because most frozen globals are inlined by the optimizer. `def` invocation is expensive, and removing any code from it helps (e.g. makes compiler inline more).

With this stack, we no longer update `module_variables`, but instead, when a module is needed, it is obtained from top frame def (which is currently evaluated def).

Don't pay for what we don't use.

Reviewed By: JakobDegen

Differential Revision: D58068387

fbshipit-source-id: 10559f3979e7ef58b1b0c4ffef6b937a643cc580

starlark-rust/starlark/src/debug/evaluate.rs

@@ -78,10 +78,8 @@ impl<'v> Evaluator<'v, '_, '_> {
             }
         }
 
-        let orig_module_variables = self.module_variables.take();
         let globals = self.top_frame_def_info_for_debugger()?.globals;
         let res = self.eval_module(statements, &globals);
-        self.module_variables = orig_module_variables;
 
         // Now put the Module back how it was before we started, as best we can
         // and move things into locals if that makes sense

starlark-rust/starlark/src/eval/compiler/def.rs

@@ -788,9 +788,9 @@ where
         if Self::FROZEN {
             debug_assert!(self.module.load_relaxed().is_some());
         }
-        let res = eval.with_function_context(me, self.module.load_relaxed(), self.bc());
 
-        res.map_err(EvalException::into_error)
+        eval.eval_bc(me, self.bc())
+            .map_err(EvalException::into_error)
     }
 
     pub(crate) fn resolve_arg_name(&self, name: Hashed<&str>) -> ResolvedArgName {

starlark-rust/starlark/src/eval/runtime/evaluator.rs

@@ -116,10 +116,6 @@ pub(crate) const DEFAULT_STACK_SIZE: usize = 50;
 pub struct Evaluator<'v, 'a, 'e> {
     // The module that is being used for this evaluation
     pub(crate) module_env: &'v Module,
-    // The module-level variables in scope at the moment.
-    // If `None` then we're in the initial module, use variables from `module_env`.
-    // If `Some` we've called a `def` in a loaded frozen module.
-    pub(crate) module_variables: Option<FrozenRef<'static, FrozenModuleData>>,
     /// Current function (`def` or `lambda`) frame: locals and bytecode stack.
     pub(crate) current_frame: BcFramePtr<'v>,
     // How we deal with a `load` function.
@@ -218,7 +214,6 @@ impl<'v, 'a, 'e: 'a> Evaluator<'v, 'a, 'e> {
         Evaluator {
             call_stack: CheapCallStack::default(),
             module_env: module,
-            module_variables: None,
             current_frame: BcFramePtr::null(),
             loader: None,
             extra: None,
@@ -471,26 +466,6 @@ impl<'v, 'a, 'e: 'a> Evaluator<'v, 'a, 'e> {
         res
     }
 
-    /// Called to change the local variables, from the callee.
-    /// Only called for user written functions.
-    #[inline(always)] // There is only one caller
-    pub(crate) fn with_function_context(
-        &mut self,
-        def: Value<'v>,
-        module: Option<FrozenRef<'static, FrozenModuleData>>, // None == use module_env
-        bc: &Bc,
-    ) -> Result<Value<'v>, EvalException> {
-        // Set up for the new function call
-        let old_module_variables = mem::replace(&mut self.module_variables, module);
-
-        // Run the computation
-        let res = self.eval_bc(def, bc);
-
-        // Restore them all back
-        self.module_variables = old_module_variables;
-        res
-    }
-
     /// The active heap where [`Value`]s are allocated.
     pub fn heap(&self) -> &'v Heap {
         self.module_env.heap()