From 610cf2c3c8bb7145829fb35ec3038365950d650d Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Tue, 20 Jun 2023 14:23:41 +0200 Subject: [PATCH] Use `Rc` instead of `Gc` for `CompileTimeEnvironment`s (#3025) * Use `Rc` instead of `Gc` for `CompileTimeEnvironment`s * Add comment --- boa_engine/src/bytecompiler/env.rs | 5 +++-- boa_engine/src/bytecompiler/function.rs | 6 ++++-- boa_engine/src/bytecompiler/mod.rs | 13 ++++++++----- boa_engine/src/environments/compile.rs | 18 ++++++++++++------ .../environments/runtime/declarative/mod.rs | 18 ++++++++++++------ boa_engine/src/environments/runtime/mod.rs | 17 ++++++++--------- boa_engine/src/module/source.rs | 4 ++-- boa_engine/src/vm/code_block.rs | 16 +++++++++++++--- 8 files changed, 62 insertions(+), 35 deletions(-) diff --git a/boa_engine/src/bytecompiler/env.rs b/boa_engine/src/bytecompiler/env.rs index 26aec1e811..ffe025a94d 100644 --- a/boa_engine/src/bytecompiler/env.rs +++ b/boa_engine/src/bytecompiler/env.rs @@ -1,12 +1,13 @@ +use std::{cell::RefCell, rc::Rc}; + use super::ByteCompiler; use crate::environments::{BindingLocator, BindingLocatorError, CompileTimeEnvironment}; use boa_ast::expression::Identifier; -use boa_gc::{Gc, GcRefCell}; impl ByteCompiler<'_, '_> { /// Push either a new declarative or function environment on the compile time environment stack. pub(crate) fn push_compile_environment(&mut self, function_scope: bool) { - self.current_environment = Gc::new(GcRefCell::new(CompileTimeEnvironment::new( + self.current_environment = Rc::new(RefCell::new(CompileTimeEnvironment::new( self.current_environment.clone(), function_scope, ))); diff --git a/boa_engine/src/bytecompiler/function.rs b/boa_engine/src/bytecompiler/function.rs index a1e055a794..cf88f6b1e3 100644 --- a/boa_engine/src/bytecompiler/function.rs +++ b/boa_engine/src/bytecompiler/function.rs @@ -1,3 +1,5 @@ +use std::{cell::RefCell, rc::Rc}; + use crate::{ builtins::function::ThisMode, bytecompiler::ByteCompiler, @@ -6,7 +8,7 @@ use crate::{ Context, }; use boa_ast::function::{FormalParameterList, FunctionBody}; -use boa_gc::{Gc, GcRefCell}; +use boa_gc::Gc; use boa_interner::Sym; /// `FunctionCompiler` is used to compile AST functions to bytecode. @@ -88,7 +90,7 @@ impl FunctionCompiler { mut self, parameters: &FormalParameterList, body: &FunctionBody, - outer_env: Gc>, + outer_env: Rc>, context: &mut Context<'_>, ) -> Gc { self.strict = self.strict || body.strict(); diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index 2b7cbf6eb9..b4274e74c9 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -11,7 +11,10 @@ mod module; mod statement; mod utils; -use std::cell::Cell; +use std::{ + cell::{Cell, RefCell}, + rc::Rc, +}; use crate::{ builtins::function::ThisMode, @@ -35,7 +38,7 @@ use boa_ast::{ pattern::Pattern, Declaration, Expression, Statement, StatementList, StatementListItem, }; -use boa_gc::{Gc, GcRefCell}; +use boa_gc::Gc; use boa_interner::{Interner, Sym}; use rustc_hash::FxHashMap; @@ -241,10 +244,10 @@ pub struct ByteCompiler<'ctx, 'host> { pub(crate) functions: Vec>, /// Compile time environments in this function. - pub(crate) compile_environments: Vec>>, + pub(crate) compile_environments: Vec>>, /// The environment that is currently active. - pub(crate) current_environment: Gc>, + pub(crate) current_environment: Rc>, pub(crate) code_block_flags: CodeBlockFlags, @@ -273,7 +276,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> { name: Sym, strict: bool, json_parse: bool, - current_environment: Gc>, + current_environment: Rc>, // TODO: remove when we separate scripts from the context context: &'ctx mut Context<'host>, ) -> ByteCompiler<'ctx, 'host> { diff --git a/boa_engine/src/environments/compile.rs b/boa_engine/src/environments/compile.rs index 8aab0a0607..5906fa9e12 100644 --- a/boa_engine/src/environments/compile.rs +++ b/boa_engine/src/environments/compile.rs @@ -1,6 +1,8 @@ +use std::{cell::RefCell, rc::Rc}; + use crate::environments::runtime::BindingLocator; use boa_ast::expression::Identifier; -use boa_gc::{Finalize, Gc, GcRefCell, Trace}; +use boa_gc::{empty_trace, Finalize, Trace}; use rustc_hash::FxHashMap; @@ -20,15 +22,19 @@ struct CompileTimeBinding { /// A compile time environment maps bound identifiers to their binding positions. /// /// A compile time environment also indicates, if it is a function environment. -#[derive(Debug, Finalize, Trace)] +#[derive(Debug, Finalize)] pub(crate) struct CompileTimeEnvironment { - outer: Option>>, + outer: Option>>, environment_index: u32, - #[unsafe_ignore_trace] bindings: FxHashMap, function_scope: bool, } +// Safety: Nothing in this struct needs tracing, so this is safe. +unsafe impl Trace for CompileTimeEnvironment { + empty_trace!(); +} + impl CompileTimeEnvironment { /// Creates a new global compile time environment. pub(crate) fn new_global() -> Self { @@ -41,7 +47,7 @@ impl CompileTimeEnvironment { } /// Creates a new compile time environment. - pub(crate) fn new(parent: Gc>, function_scope: bool) -> Self { + pub(crate) fn new(parent: Rc>, function_scope: bool) -> Self { let index = parent.borrow().environment_index + 1; Self { outer: Some(parent), @@ -291,7 +297,7 @@ impl CompileTimeEnvironment { } /// Gets the outer environment of this environment. - pub(crate) fn outer(&self) -> Option>> { + pub(crate) fn outer(&self) -> Option>> { self.outer.clone() } diff --git a/boa_engine/src/environments/runtime/declarative/mod.rs b/boa_engine/src/environments/runtime/declarative/mod.rs index 97f65b1b9b..5e2a804fba 100644 --- a/boa_engine/src/environments/runtime/declarative/mod.rs +++ b/boa_engine/src/environments/runtime/declarative/mod.rs @@ -3,9 +3,12 @@ mod global; mod lexical; mod module; -use std::cell::Cell; +use std::{ + cell::{Cell, RefCell}, + rc::Rc, +}; -use boa_gc::{Finalize, Gc, GcRefCell, Trace}; +use boa_gc::{Finalize, GcRefCell, Trace}; pub(crate) use function::{FunctionEnvironment, FunctionSlots, ThisBindingStatus}; pub(crate) use global::GlobalEnvironment; pub(crate) use lexical::LexicalEnvironment; @@ -36,7 +39,10 @@ use crate::{environments::CompileTimeEnvironment, JsObject, JsResult, JsValue}; #[derive(Debug, Trace, Finalize)] pub(crate) struct DeclarativeEnvironment { kind: DeclarativeEnvironmentKind, - compile: Gc>, + + // Safety: Nothing in CompileTimeEnvironment needs tracing. + #[unsafe_ignore_trace] + compile: Rc>, } impl DeclarativeEnvironment { @@ -44,20 +50,20 @@ impl DeclarativeEnvironment { pub(crate) fn global(global_this: JsObject) -> Self { Self { kind: DeclarativeEnvironmentKind::Global(GlobalEnvironment::new(global_this)), - compile: Gc::new(GcRefCell::new(CompileTimeEnvironment::new_global())), + compile: Rc::new(RefCell::new(CompileTimeEnvironment::new_global())), } } /// Creates a new `DeclarativeEnvironment` from its kind and compile environment. pub(crate) fn new( kind: DeclarativeEnvironmentKind, - compile: Gc>, + compile: Rc>, ) -> Self { Self { kind, compile } } /// Gets the compile time environment of this environment. - pub(crate) fn compile_env(&self) -> Gc> { + pub(crate) fn compile_env(&self) -> Rc> { self.compile.clone() } diff --git a/boa_engine/src/environments/runtime/mod.rs b/boa_engine/src/environments/runtime/mod.rs index 2cb196ae59..0b09a4f06e 100644 --- a/boa_engine/src/environments/runtime/mod.rs +++ b/boa_engine/src/environments/runtime/mod.rs @@ -1,10 +1,12 @@ +use std::{cell::RefCell, rc::Rc}; + use crate::{ environments::CompileTimeEnvironment, object::{JsObject, PrivateName}, Context, JsResult, JsString, JsSymbol, JsValue, }; use boa_ast::expression::Identifier; -use boa_gc::{empty_trace, Finalize, Gc, GcRefCell, Trace}; +use boa_gc::{empty_trace, Finalize, Gc, Trace}; use rustc_hash::FxHashSet; mod declarative; @@ -220,7 +222,7 @@ impl EnvironmentStack { #[track_caller] pub(crate) fn push_lexical( &mut self, - compile_environment: Gc>, + compile_environment: Rc>, ) -> u32 { let num_bindings = compile_environment.borrow().num_bindings(); @@ -265,7 +267,7 @@ impl EnvironmentStack { #[track_caller] pub(crate) fn push_function( &mut self, - compile_environment: Gc>, + compile_environment: Rc>, function_slots: FunctionSlots, ) { let num_bindings = compile_environment.borrow().num_bindings(); @@ -309,7 +311,7 @@ impl EnvironmentStack { #[track_caller] pub(crate) fn push_function_inherit( &mut self, - compile_environment: Gc>, + compile_environment: Rc>, ) { let num_bindings = compile_environment.borrow().num_bindings(); @@ -361,10 +363,7 @@ impl EnvironmentStack { /// /// Panics if no environment exists on the stack. #[track_caller] - pub(crate) fn push_module( - &mut self, - compile_environment: Gc>, - ) { + pub(crate) fn push_module(&mut self, compile_environment: Rc>) { let num_bindings = compile_environment.borrow().num_bindings(); self.stack.push(Environment::Declarative(Gc::new( DeclarativeEnvironment::new( @@ -401,7 +400,7 @@ impl EnvironmentStack { /// # Panics /// /// Panics if no environment exists on the stack. - pub(crate) fn current_compile_environment(&self) -> Gc> { + pub(crate) fn current_compile_environment(&self) -> Rc> { self.stack .iter() .filter_map(Environment::as_declarative) diff --git a/boa_engine/src/module/source.rs b/boa_engine/src/module/source.rs index 92337e6725..d73a26ee01 100644 --- a/boa_engine/src/module/source.rs +++ b/boa_engine/src/module/source.rs @@ -1,5 +1,5 @@ use std::{ - cell::Cell, + cell::{Cell, RefCell}, collections::HashSet, hash::{BuildHasherDefault, Hash}, rc::Rc, @@ -1403,7 +1403,7 @@ impl SourceTextModule { // 6. Set module.[[Environment]] to env. let global_env = realm.environment().clone(); let global_compile_env = global_env.compile_env(); - let module_compile_env = Gc::new(GcRefCell::new(CompileTimeEnvironment::new( + let module_compile_env = Rc::new(RefCell::new(CompileTimeEnvironment::new( global_compile_env, true, ))); diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 36765072bb..d0cd9ad397 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -20,10 +20,15 @@ use crate::{ }; use bitflags::bitflags; use boa_ast::function::FormalParameterList; -use boa_gc::{empty_trace, Finalize, Gc, GcRefCell, Trace}; +use boa_gc::{empty_trace, Finalize, Gc, Trace}; use boa_interner::Sym; use boa_profiler::Profiler; -use std::{cell::Cell, collections::VecDeque, mem::size_of}; +use std::{ + cell::{Cell, RefCell}, + collections::VecDeque, + mem::size_of, + rc::Rc, +}; use thin_vec::ThinVec; #[cfg(any(feature = "trace", feature = "flowgraph"))] @@ -129,7 +134,12 @@ pub struct CodeBlock { pub(crate) functions: Box<[Gc]>, /// Compile time environments in this function. - pub(crate) compile_environments: Box<[Gc>]>, + /// + // Safety: Nothing in CompileTimeEnvironment needs tracing, so this is safe. + // + // TODO(#3034): Maybe changing this to Gc after garbage collection would be better than Rc. + #[unsafe_ignore_trace] + pub(crate) compile_environments: Box<[Rc>]>, } /// ---- `CodeBlock` public API ----