Skip to content

Commit

Permalink
Removed some unsafe_empty_trace!() calls to improve performance (#2233)
Browse files Browse the repository at this point in the history
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request fixes #1615.

It changes the following:

- Removes the `Trace` implementation from types that don't need it (except for `JsSymbol` and `JsString`, which are needed elsewere).
- Uses `#[unsafe_ignore_trace]` in places where we need to implement `Trace` for part of a structure.
- Implements a custom `Trace` in enums where deriving it is not possible, since `#[unsafe_ignore_trace]` doesn't work for enums.
  • Loading branch information
Razican committed Aug 15, 2022
1 parent 2dcdf51 commit 8843918
Show file tree
Hide file tree
Showing 19 changed files with 87 additions and 132 deletions.
9 changes: 1 addition & 8 deletions boa_engine/src/bigint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! This module implements the JavaScript bigint primitive rust type.
use crate::{builtins::Number, Context, JsValue};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use num_integer::Integer;
use num_traits::{pow::Pow, FromPrimitive, One, ToPrimitive, Zero};
use std::{
Expand All @@ -18,17 +17,11 @@ use serde::{Deserialize, Serialize};

/// JavaScript bigint primitive rust type.
#[cfg_attr(feature = "deser", derive(Serialize, Deserialize))]
#[derive(Debug, Finalize, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct JsBigInt {
inner: Rc<RawBigInt>,
}

// Safety: BigInt does not contain any objects which needs to be traced,
// so this is safe.
unsafe impl Trace for JsBigInt {
unsafe_empty_trace!();
}

impl JsBigInt {
/// Create a new [`JsBigInt`].
#[inline]
Expand Down
1 change: 1 addition & 0 deletions boa_engine/src/builtins/array/array_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use boa_profiler::Profiler;
pub struct ArrayIterator {
array: JsObject,
next_index: u64,
#[unsafe_ignore_trace]
kind: PropertyNameKind,
done: bool,
}
Expand Down
9 changes: 1 addition & 8 deletions boa_engine/src/builtins/date/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use crate::{
value::{JsValue, PreferredType},
Context, JsResult, JsString,
};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_profiler::Profiler;
use chrono::{prelude::*, Duration, LocalResult};
use std::fmt::Display;
Expand Down Expand Up @@ -61,7 +60,7 @@ macro_rules! getter_method {
}};
}

#[derive(Debug, Finalize, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct Date(Option<NaiveDateTime>);

impl Display for Date {
Expand All @@ -73,12 +72,6 @@ impl Display for Date {
}
}

unsafe impl Trace for Date {
// Date is a stack value, it doesn't require tracing.
// only safe if `chrono` never implements `Trace` for `NaiveDateTime`
unsafe_empty_trace!();
}

impl Default for Date {
fn default() -> Self {
Self(Some(Utc::now().naive_utc()))
Expand Down
3 changes: 1 addition & 2 deletions boa_engine/src/builtins/function/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ pub enum ClassFieldDefinition {
unsafe impl Trace for ClassFieldDefinition {
custom_trace! {this, {
match this {
Self::Public(key, func) => {
mark(key);
Self::Public(_key, func) => {
mark(func);
}
Self::Private(_, func) => {
Expand Down
1 change: 1 addition & 0 deletions boa_engine/src/builtins/map/map_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use boa_profiler::Profiler;
pub struct MapIterator {
iterated_map: Option<JsObject>,
map_next_index: usize,
#[unsafe_ignore_trace]
map_iteration_kind: PropertyNameKind,
lock: MapLock,
}
Expand Down
8 changes: 1 addition & 7 deletions boa_engine/src/builtins/regexp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use crate::{
value::{IntegerOrInfinity, JsValue},
Context, JsResult, JsString,
};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_profiler::Profiler;
use regress::Regex;
use std::str::FromStr;
Expand All @@ -36,7 +35,7 @@ use tap::{Conv, Pipe};
mod tests;

/// The internal representation on a `RegExp` object.
#[derive(Debug, Clone, Finalize)]
#[derive(Debug, Clone)]
pub struct RegExp {
/// Regex matcher.
matcher: Regex,
Expand All @@ -45,11 +44,6 @@ pub struct RegExp {
original_flags: JsString,
}

// Only safe while regress::Regex doesn't implement Trace itself.
unsafe impl Trace for RegExp {
unsafe_empty_trace!();
}

impl BuiltIn for RegExp {
const NAME: &'static str = "RegExp";

Expand Down
1 change: 1 addition & 0 deletions boa_engine/src/builtins/set/set_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use boa_profiler::Profiler;
pub struct SetIterator {
iterated_set: JsValue,
next_index: usize,
#[unsafe_ignore_trace]
iteration_kind: PropertyNameKind,
}

Expand Down
10 changes: 3 additions & 7 deletions boa_engine/src/builtins/typed_array/integer_indexed_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,20 @@ use crate::{
object::{JsObject, ObjectData},
Context,
};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_gc::{Finalize, Trace};

/// Type of the array content.
#[derive(Debug, Clone, Copy, Finalize, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub(crate) enum ContentType {
Number,
BigInt,
}

unsafe impl Trace for ContentType {
// safe because `ContentType` is `Copy`
unsafe_empty_trace!();
}

/// <https://tc39.es/ecma262/#integer-indexed-exotic-object>
#[derive(Debug, Clone, Trace, Finalize)]
pub struct IntegerIndexed {
viewed_array_buffer: Option<JsObject>,
#[unsafe_ignore_trace]
typed_array_name: TypedArrayKind,
byte_offset: u64,
byte_length: u64,
Expand Down
8 changes: 1 addition & 7 deletions boa_engine/src/builtins/typed_array/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ use crate::{
value::{IntegerOrInfinity, JsValue},
Context, JsResult, JsString,
};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_profiler::Profiler;
use num_traits::{Signed, Zero};
use std::cmp::Ordering;
Expand Down Expand Up @@ -3388,7 +3387,7 @@ impl TypedArray {
}

/// Names of all the typed arrays.
#[derive(Debug, Clone, Copy, Finalize, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub(crate) enum TypedArrayKind {
Int8,
Uint8,
Expand All @@ -3403,11 +3402,6 @@ pub(crate) enum TypedArrayKind {
Float64,
}

unsafe impl Trace for TypedArrayKind {
// Safe because `TypedArrayName` is `Copy`
unsafe_empty_trace!();
}

impl TypedArrayKind {
/// Gets the element size of the given typed array name, as per the [spec].
///
Expand Down
42 changes: 40 additions & 2 deletions boa_engine/src/object/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use crate::{
Context, JsBigInt, JsResult, JsString, JsSymbol, JsValue,
};

use boa_gc::{Finalize, Trace};
use boa_gc::{custom_trace, Finalize, Trace};
use boa_interner::Sym;
use rustc_hash::FxHashMap;
use std::{
Expand Down Expand Up @@ -163,7 +163,7 @@ pub struct ObjectData {
}

/// Defines the different types of objects.
#[derive(Debug, Trace, Finalize)]
#[derive(Debug, Finalize)]
pub enum ObjectKind {
AsyncGenerator(AsyncGenerator),
AsyncGeneratorFunction(Function),
Expand Down Expand Up @@ -201,6 +201,44 @@ pub enum ObjectKind {
Promise(Promise),
}

unsafe impl Trace for ObjectKind {
custom_trace! {this, {
match this {
Self::ArrayIterator(i) => mark(i),
Self::ArrayBuffer(b) => mark(b),
Self::Map(m) => mark(m),
Self::MapIterator(i) => mark(i),
Self::RegExpStringIterator(i) => mark(i),
Self::DataView(v) => mark(v),
Self::ForInIterator(i) => mark(i),
Self::Function(f) | Self::GeneratorFunction(f) => mark(f),
Self::BoundFunction(f) => mark(f),
Self::Generator(g) => mark(g),
Self::Set(s) => mark(s),
Self::SetIterator(i) => mark(i),
Self::StringIterator(i) => mark(i),
Self::Proxy(p) => mark(p),
Self::Arguments(a) => mark(a),
Self::NativeObject(o) => mark(o),
Self::IntegerIndexed(i) => mark(i),
#[cfg(feature = "intl")]
Self::DateTimeFormat(f) => mark(f),
Self::Promise(p) => mark(p),
Self::RegExp(_)
| Self::BigInt(_)
| Self::Boolean(_)
| Self::String(_)
| Self::Date(_)
| Self::Array
| Self::Error
| Self::Ordinary
| Self::Global
| Self::Number(_)
| Self::Symbol(_) => {}
}
}}
}

impl ObjectData {
/// Create the `AsyncGenerator` object data
pub fn async_generator(async_generator: AsyncGenerator) -> Self {
Expand Down
11 changes: 0 additions & 11 deletions boa_engine/src/property/attribute/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! This module implements the `Attribute` struct which contains the attibutes for property descriptors.
use bitflags::bitflags;
use boa_gc::{unsafe_empty_trace, Finalize, Trace};

#[cfg(test)]
mod tests;
Expand All @@ -16,7 +15,6 @@ bitflags! {
/// - `[[Configurable]]` (`CONFIGURABLE`) - If `false`, attempts to delete the property,
/// change the property to be an `accessor property`, or change its attributes (other than `[[Value]]`,
/// or changing `[[Writable]]` to `false`) will fail.
#[derive(Finalize)]
pub struct Attribute: u8 {
/// The `Writable` attribute decides whether the value associated with the property can be changed or not, from its initial value.
const WRITABLE = 0b0000_0001;
Expand All @@ -38,15 +36,6 @@ bitflags! {
}
}

// We implement `Trace` manualy rather that wih derive, beacuse `rust-gc`,
// derive `Trace` does not allow `Copy` and `Trace` to be both implemented.
//
// SAFETY: The `Attribute` struct only contains an `u8`
// and therefore it should be safe to implement an empty trace.
unsafe impl Trace for Attribute {
unsafe_empty_trace!();
}

impl Attribute {
/// Clear all flags.
#[inline]
Expand Down
10 changes: 3 additions & 7 deletions boa_engine/src/property/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
//! [section]: https://tc39.es/ecma262/#sec-property-attributes
use crate::{JsString, JsSymbol, JsValue};
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_gc::{Finalize, Trace};
use std::fmt;

mod attribute;
Expand Down Expand Up @@ -488,7 +488,7 @@ impl From<PropertyDescriptorBuilder> for PropertyDescriptor {
/// - [ECMAScript reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-ispropertykey
#[derive(Trace, Finalize, PartialEq, Debug, Clone, Eq, Hash)]
#[derive(PartialEq, Debug, Clone, Eq, Hash)]
pub enum PropertyKey {
String(JsString),
Symbol(JsSymbol),
Expand Down Expand Up @@ -673,13 +673,9 @@ impl PartialEq<&str> for PropertyKey {
}
}

#[derive(Debug, Clone, Copy, Finalize)]
#[derive(Debug, Clone, Copy)]
pub(crate) enum PropertyNameKind {
Key,
Value,
KeyAndValue,
}

unsafe impl Trace for PropertyNameKind {
unsafe_empty_trace!();
}
12 changes: 6 additions & 6 deletions boa_engine/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,12 @@ pub struct JsString {
_marker: PhantomData<Rc<str>>,
}

// Safety: JsString does not contain any objects which needs to be traced,
// so this is safe.
unsafe impl Trace for JsString {
unsafe_empty_trace!();
}

/// This struct uses a technique called tagged pointer to benefit from the fact that newly
/// allocated pointers are always word aligned on 64-bits platforms, making it impossible
/// to have a LSB equal to 1. More details about this technique on the article of Wikipedia
Expand Down Expand Up @@ -942,12 +948,6 @@ impl JsString {
}
}

// Safety: [`JsString`] does not contain any objects which recquire trace,
// so this is safe.
unsafe impl Trace for JsString {
unsafe_empty_trace!();
}

impl Clone for JsString {
#[inline]
fn clone(&self) -> Self {
Expand Down
16 changes: 7 additions & 9 deletions boa_engine/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,17 @@ struct Inner {
}

/// This represents a JavaScript symbol primitive.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Finalize)]
pub struct JsSymbol {
inner: Rc<Inner>,
}

// Safety: JsSymbol does not contain any objects which needs to be traced,
// so this is safe.
unsafe impl Trace for JsSymbol {
unsafe_empty_trace!();
}

impl JsSymbol {
/// Create a new symbol.
#[inline]
Expand Down Expand Up @@ -301,14 +307,6 @@ impl JsSymbol {
}
}

impl Finalize for JsSymbol {}

// Safety: `JsSymbol` does not contain any object that require trace,
// so this is safe.
unsafe impl Trace for JsSymbol {
unsafe_empty_trace!();
}

impl Display for JsSymbol {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Expand Down
9 changes: 1 addition & 8 deletions boa_engine/src/syntax/ast/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
//! [spec]: https://tc39.es/ecma262/#sec-primary-expression-literals
//! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Grammar_and_types#Literals
use boa_gc::{unsafe_empty_trace, Finalize, Trace};
use boa_interner::{Interner, Sym, ToInternedString};
use num_bigint::BigInt;
#[cfg(feature = "deser")]
Expand All @@ -24,7 +23,7 @@ use serde::{Deserialize, Serialize};
/// [spec]: https://tc39.es/ecma262/#sec-primary-expression-literals
/// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Grammar_and_types#Literals
#[cfg_attr(feature = "deser", derive(Serialize, Deserialize))]
#[derive(Clone, Debug, Finalize, PartialEq)]
#[derive(Clone, Debug, PartialEq)]
pub enum Const {
/// A string literal is zero or more characters enclosed in double (`"`) or single (`'`) quotation marks.
///
Expand Down Expand Up @@ -112,12 +111,6 @@ pub enum Const {
Undefined,
}

// Safety: Const does not contain any objects which needs to be traced,
// so this is safe.
unsafe impl Trace for Const {
unsafe_empty_trace!();
}

impl From<Sym> for Const {
fn from(string: Sym) -> Self {
Self::String(string)
Expand Down
Loading

0 comments on commit 8843918

Please sign in to comment.