Skip to content

perf(bytecode): elide arguments object instructions when its never used #686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion nova_vm/src/ecmascript/builtins/ecmascript_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use core::{

use oxc_ast::ast::{FormalParameters, FunctionBody};
use oxc_ecmascript::IsSimpleParameterList;
use oxc_semantic::Semantic;
use oxc_span::Span;

use crate::{
Expand All @@ -26,7 +27,8 @@ use crate::{
},
scripts_and_modules::{ScriptOrModule, source_code::SourceCode},
syntax_directed_operations::function_definitions::{
evaluate_async_function_body, evaluate_function_body, evaluate_generator_body,
CompileFunctionBodyData, evaluate_async_function_body, evaluate_function_body,
evaluate_generator_body,
},
types::{
BUILTIN_STRING_MEMORY, ECMAScriptFunctionHeapData, Function,
Expand Down Expand Up @@ -271,6 +273,39 @@ impl IndexMut<ECMAScriptFunction<'_>> for Vec<Option<ECMAScriptFunctionHeapData<
}

impl<'a> ECMAScriptFunction<'a> {
pub(crate) fn get_compile_data(
self,
agent: &Agent,
_gc: NoGcScope<'a, '_>,
) -> CompileFunctionBodyData<'a> {
let ecmascript_function = &agent[self].ecmascript_function;
// SAFETY: We're alive so SourceCode must be too.
let (params, body) = unsafe {
(
ecmascript_function.formal_parameters.as_ref(),
ecmascript_function.ecmascript_code.as_ref(),
)
};
CompileFunctionBodyData {
params,
body,
is_strict: ecmascript_function.strict,
is_lexical: ecmascript_function.this_mode == ThisMode::Lexical,
is_concise_body: ecmascript_function.is_concise_arrow_function,
}
}

pub(crate) fn get_semantic(
self,
agent: &Agent,
gc: NoGcScope<'a, '_>,
) -> &'a Semantic<'static> {
agent[self]
.ecmascript_function
.source_code
.get_semantic(agent, gc)
}

pub(crate) const fn _def() -> Self {
ECMAScriptFunction(ECMAScriptFunctionIndex::from_u32_index(0))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use std::hint::unreachable_unchecked;

use oxc_span::SourceType;

use crate::{
Expand All @@ -14,7 +16,7 @@ use crate::{
ordinary::get_prototype_from_constructor, ordinary_function_create, set_function_name,
},
execution::{Agent, Environment, JsResult, ProtoIntrinsics, Realm, agent::ExceptionType},
scripts_and_modules::source_code::{SourceCode, SourceCodeHeapData},
scripts_and_modules::source_code::SourceCode,
types::{
BUILTIN_STRING_MEMORY, Function, IntoObject, IntoValue, Object, Primitive, String,
Value,
Expand Down Expand Up @@ -108,14 +110,18 @@ impl DynamicFunctionKind {
DynamicFunctionKind::AsyncGenerator => "async function*",
}
}
fn function_matches_kind(&self, function: &oxc_ast::ast::Function) -> bool {
let (is_async, is_generator) = match self {
DynamicFunctionKind::Normal => (false, false),
DynamicFunctionKind::Generator => (false, true),
DynamicFunctionKind::Async => (true, false),
DynamicFunctionKind::AsyncGenerator => (true, true),
};
function.r#async == is_async && function.generator == is_generator
fn statement_matches_function_kind(&self, function: &oxc_ast::ast::Statement) -> bool {
if let oxc_ast::ast::Statement::FunctionDeclaration(function) = function {
let (is_async, is_generator) = match self {
DynamicFunctionKind::Normal => (false, false),
DynamicFunctionKind::Generator => (false, true),
DynamicFunctionKind::Async => (true, false),
DynamicFunctionKind::AsyncGenerator => (true, true),
};
function.r#async == is_async && function.generator == is_generator
} else {
false
}
}
fn intrinsic_prototype(&self) -> ProtoIntrinsics {
match self {
Expand Down Expand Up @@ -234,75 +240,57 @@ pub(crate) fn create_dynamic_function<'a>(
// avoid code injection, but oxc doesn't have a public API to do that.
// Instead, we parse the source string as a script, and throw unless it has
// exactly one statement which is a function declaration of the right kind.
let (function, source_code) = {
let mut function = None;
let mut source_code = None;

let source_code = {
let source_type = SourceType::default().with_script(true);
// SAFETY: The safety requirements are that the SourceCode cannot be
// GC'd before the program is dropped. If this function returns
// successfully, then the program's AST and the SourceCode will both be
// kept alive in the returned function object.
let parsed_result =
unsafe { SourceCode::parse_source(agent, source_string, source_type, gc.nogc()) };

if let Ok((program, sc)) = parsed_result {
source_code = Some(sc);
if program.hashbang.is_none()
&& program.directives.is_empty()
&& program.body.len() == 1
{
if let oxc_ast::ast::Statement::FunctionDeclaration(funct) = &program.body[0] {
if kind.function_matches_kind(funct) {
// SAFETY: the Function is inside a oxc_allocator::Box, which will remain
// alive as long as `source_code` is kept alive. Similarly, the inner
// lifetime of Function is also kept alive by `source_code`.`
function = Some(unsafe {
core::mem::transmute::<
&oxc_ast::ast::Function,
&'static oxc_ast::ast::Function,
>(funct)
});
}
match SourceCode::parse_source(agent, source_string, source_type, gc.nogc()) {
Ok(sc) => {
let program = sc.get_program(agent, gc.nogc());
if !(program.hashbang.is_none()
&& program.directives.is_empty()
&& program.body.len() == 1
&& kind.statement_matches_function_kind(&program.body[0]))
{
return Err(agent.throw_exception_with_static_message(
ExceptionType::SyntaxError,
"Invalid function source text: Did not form a single function statement",
gc.into_nogc(),
));
}
sc
}
}

if let Some(function) = function {
(function, source_code.unwrap())
} else {
if source_code.is_some() {
// In this branch, since we're not returning the function, we
// know `source_code` won't be reachable from any heap object,
// so we pop it off the heap to help GC along.
agent.heap.alloc_counter = agent
.heap
.alloc_counter
.saturating_sub(core::mem::size_of::<Option<SourceCodeHeapData<'static>>>());
agent.heap.source_codes.pop();
debug_assert_eq!(
source_code.unwrap().get_index(),
agent.heap.source_codes.len()
);
Err(err) => {
let error_message = format!("Invalid function source text: {}", err[0].message);
return Err(agent.throw_exception(
ExceptionType::SyntaxError,
error_message,
gc.into_nogc(),
));
}
return Err(agent.throw_exception_with_static_message(
ExceptionType::SyntaxError,
"Invalid function source text.",
gc.into_nogc(),
));
}
};

let source_code = source_code.scope(agent, gc.nogc());
let function_prototype = get_prototype_from_constructor(
agent,
constructor.unbind(),
kind.intrinsic_prototype(),
gc.reborrow(),
)
.unbind()?;
let gc = gc.into_nogc();
let function_prototype = function_prototype.bind(gc);
let program = source_code.get(agent).get_program(agent, gc);
let Some(oxc_ast::ast::Statement::FunctionDeclaration(function)) = program.body.first() else {
// SAFETY: We checked that the function declaration matches our
// expectations before the getting the prototype from our constructor.
// That action can trigger GC, but we've scoped source_code so our
// Program hasn't been GC'd (this we know statically) but most
// importantly: there is no way that user code could have manipulated
// our AST, so our previous checks still apply.
unsafe { unreachable_unchecked() }
};
let params = OrdinaryFunctionCreateParams {
function_prototype: get_prototype_from_constructor(
agent,
constructor.unbind(),
kind.intrinsic_prototype(),
gc.reborrow(),
)
.unbind()?
.bind(gc.nogc()),
function_prototype,
// SAFETY: source_code was not shared.
source_code: Some(unsafe { source_code.take(agent) }),
source_text: function.span,
Expand All @@ -318,13 +306,11 @@ pub(crate) fn create_dynamic_function<'a>(
.global_env
.unwrap()
.unbind()
.bind(gc.nogc()),
.bind(gc),
),
private_env: None,
};
let f = ordinary_function_create(agent, params, gc.nogc()).unbind();
let gc = gc.into_nogc();
let f = f.bind(gc);
let f = ordinary_function_create(agent, params, gc);

set_function_name(
agent,
Expand Down
32 changes: 20 additions & 12 deletions nova_vm/src/ecmascript/builtins/global_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use ahash::AHashSet;
use oxc_ast::ast::{BindingIdentifier, Program, VariableDeclarationKind};
use oxc_ecmascript::BoundNames;
use oxc_semantic::Semantic;
use oxc_span::SourceType;

use crate::ecmascript::abstract_operations::type_conversion::{
Expand Down Expand Up @@ -215,17 +216,10 @@ pub fn perform_eval<'gc>(
} else {
SourceType::default().with_script(true)
};
// SAFETY: Script is only kept alive for the duration of this call, and any
// references made to it by functions being created in the eval call will
// take a copy of the SourceCode. The SourceCode is also kept in the
// evaluation context and thus cannot be garbage collected while the eval
// call happens.
// The Program thus refers to a valid, live Allocator for the duration of
// this call.
let parse_result = unsafe { SourceCode::parse_source(agent, x, source_type, gc.nogc()) };
let parse_result = SourceCode::parse_source(agent, x, source_type, gc.nogc());

// b. If script is a List of errors, throw a SyntaxError exception.
let Ok((script, source_code)) = parse_result else {
let Ok(source_code) = parse_result else {
// TODO: Include error messages in the exception.
return Err(agent.throw_exception_with_static_message(
ExceptionType::SyntaxError,
Expand All @@ -234,6 +228,9 @@ pub fn perform_eval<'gc>(
));
};

let script = source_code.get_program(agent, gc.nogc());
let semantic = source_code.get_semantic(agent, gc.nogc());

// c. If script Contains ScriptBody is false, return undefined.
if script.is_empty() {
return Ok(Value::Undefined);
Expand Down Expand Up @@ -330,10 +327,21 @@ pub fn perform_eval<'gc>(
// 27. Push evalContext onto the execution context stack; evalContext is now the running execution context.
agent.push_execution_context(eval_context);

// SAFETY: We've pushed eval_context onto the execution context stack. The
// context holds our SourceCode and keeps it from being GC'd, meaning that
// the Program and Semantic will be kept alive for the duration of the
// eval_declaration_instantiation call.
let (script, semantic) = unsafe {
(
core::mem::transmute::<&Program, &'static Program<'static>>(script),
core::mem::transmute::<&Semantic, &'static Semantic<'static>>(semantic),
)
};

// 28. Let result be Completion(EvalDeclarationInstantiation(body, varEnv, lexEnv, privateEnv, strictEval)).
let result = eval_declaration_instantiation(
agent,
&script,
script,
ecmascript_code.variable_environment,
ecmascript_code.lexical_environment,
ecmascript_code.private_environment,
Expand All @@ -346,8 +354,8 @@ pub fn perform_eval<'gc>(
// 29. If result is a normal completion, then
let result = match result {
Ok(_) => {
let exe =
Executable::compile_eval_body(agent, &script, gc.nogc()).scope(agent, gc.nogc());
let exe = Executable::compile_eval_body(agent, script, semantic, gc.nogc())
.scope(agent, gc.nogc());
// a. Set result to Completion(Evaluation of body).
// 30. If result is a normal completion and result.[[Value]] is empty, then
// a. Set result to NormalCompletion(undefined).
Expand Down
28 changes: 20 additions & 8 deletions nova_vm/src/ecmascript/scripts_and_modules/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ use ahash::AHashSet;
use core::{
any::Any,
marker::PhantomData,
mem::ManuallyDrop,
ops::{Index, IndexMut},
};
use oxc_ast::ast::{BindingIdentifier, Program, VariableDeclarationKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_ecmascript::BoundNames;
use oxc_span::SourceType;
use std::ptr::NonNull;

use super::source_code::SourceCode;

Expand All @@ -53,7 +53,11 @@ impl core::fmt::Debug for Script<'_> {
}
}

impl Script<'_> {
impl<'a> Script<'a> {
pub(crate) fn get_source_code(self, agent: &Agent) -> SourceCode<'a> {
agent[self].source_code
}

/// Creates a script identififer from a usize.
///
/// ## Panics
Expand Down Expand Up @@ -147,7 +151,7 @@ pub struct ScriptRecord<'a> {
/// allocator drops all of the data in a single go. All that needs to be
/// dropped here is the local Program itself, not any of its referred
/// parts.
pub(crate) ecmascript_code: ManuallyDrop<Program<'static>>,
pub(crate) ecmascript_code: NonNull<Program<'static>>,

/// ### \[\[LoadedModules]]
///
Expand Down Expand Up @@ -281,9 +285,9 @@ pub fn parse_script<'a>(

// SAFETY: Script keeps the SourceCode reference alive in the Heap, thus
// making the Program's references point to a live Allocator.
let parse_result = unsafe { SourceCode::parse_source(agent, source_text, source_type, gc) };
let parse_result = SourceCode::parse_source(agent, source_text, source_type, gc);

let (program, source_code) = match parse_result {
let source_code = match parse_result {
// 2. If script is a List of errors, return script.
Ok(result) => result,
Err(errors) => {
Expand All @@ -296,7 +300,7 @@ pub fn parse_script<'a>(
// [[Realm]]: realm,
realm: realm.unbind(),
// [[ECMAScriptCode]]: script,
ecmascript_code: ManuallyDrop::new(program),
ecmascript_code: source_code.get_program_pointer(agent),
// [[LoadedModules]]: « »,
loaded_modules: (),
// [[HostDefined]]: hostDefined,
Expand All @@ -321,7 +325,15 @@ pub fn script_evaluation<'a>(
let script = script.bind(gc.nogc());
let script_record = &agent[script];
let realm_id = script_record.realm;
let is_strict_mode = script_record.ecmascript_code.source_type.is_strict();
// SAFETY: Script is currently alive, meaning that the SourceCode is
// currently alive, and thus the Program pointer is still valid.
let is_strict_mode = unsafe {
script_record
.ecmascript_code
.as_ref()
.source_type
.is_strict()
};
let source_code = script_record.source_code;
let realm = agent.get_realm_record_by_id(realm_id);

Expand Down Expand Up @@ -442,7 +454,7 @@ pub(crate) fn global_declaration_instantiation<'a>(
// long as the Script is alive in the heap as they are not reallocated.
// Thus in effect VarScopedDeclaration<'_> is valid for the duration
// of the global_declaration_instantiation call.
let script = unsafe { core::mem::transmute::<&Program, &'static Program<'static>>(script) };
let script = unsafe { script.as_ref() };
// 1. Let lexNames be the LexicallyDeclaredNames of script.
let lex_names = script_lexically_declared_names(script);
// 2. Let varNames be the VarDeclaredNames of script.
Expand Down
Loading
Loading