Skip to content

Commit

Permalink
Remove unneeded Option in property_table
Browse files Browse the repository at this point in the history
  • Loading branch information
@HalidOdat
HalidOdat committed Mar 24, 2023
1 parent 649150e commit 42fe677
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 75 deletions.
23 changes: 8 additions & 15 deletions boa_engine/src/object/property_map.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,11 @@ impl PropertyMap {
let mut builder = PropertyDescriptor::builder()
.configurable(attributes.contains(PropertyDescriptorAttribute::CONFIGURABLE))
.enumerable(attributes.contains(PropertyDescriptorAttribute::ENUMERABLE));
if attributes.contains(PropertyDescriptorAttribute::HAS_GET)
|| attributes.contains(PropertyDescriptorAttribute::HAS_SET)
{
if attributes.contains(PropertyDescriptorAttribute::HAS_GET) {
if attributes.is_accessor_descriptor() {
if attributes.has_get() {
builder = builder.get(self.storage[index].clone());
}
if attributes.contains(PropertyDescriptorAttribute::HAS_SET) {
if attributes.has_set() {
builder = builder.set(self.storage[index + 1].clone());
}
} else {
Expand Down Expand Up @@ -336,21 +334,18 @@ impl PropertyMap {
property_key: key.clone(),
attributes,
};
self.shape = self.shape.change_attributes_transition(key);
self.shape = self.shape.change_attributes_transition(key, slot.index);
}

// TODO: maybe implement is_accessor/data_descriptor on attributes?
if attributes.contains(PropertyDescriptorAttribute::HAS_GET)
|| attributes.contains(PropertyDescriptorAttribute::HAS_SET)
{
if attributes.contains(PropertyDescriptorAttribute::HAS_GET) {
if attributes.is_accessor_descriptor() {
if attributes.has_get() {
self.storage[index] = property
.get()
.cloned()
.map(JsValue::new)
.unwrap_or_default();
}
if attributes.contains(PropertyDescriptorAttribute::HAS_SET) {
if attributes.has_set() {
self.storage[index + 1] = property
.set()
.cloned()
Expand All @@ -369,9 +364,7 @@ impl PropertyMap {
attributes,
};
self.shape = self.shape.insert_property_transition(transition_key);
if attributes.contains(PropertyDescriptorAttribute::HAS_GET)
|| attributes.contains(PropertyDescriptorAttribute::HAS_SET)
{
if attributes.is_accessor_descriptor() {
self.storage.push(
property
.get()
Expand Down
18 changes: 17 additions & 1 deletion boa_engine/src/object/shape/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,22 @@ bitflags! {
}
}

impl PropertyDescriptorAttribute {
pub const fn is_accessor_descriptor(self) -> bool {
self.contains(Self::HAS_GET) || self.contains(Self::HAS_SET)
}
pub const fn is_data_descriptor(self) -> bool {
!self.is_accessor_descriptor()
}

pub const fn has_get(self) -> bool {
self.contains(Self::HAS_GET)
}
pub const fn has_set(self) -> bool {
self.contains(Self::HAS_SET)
}
}

#[derive(Debug, Finalize, Clone, PartialEq, Eq, Hash)]
pub(crate) struct TransitionKey {
pub(crate) property_key: PropertyKey,
Expand Down Expand Up @@ -118,7 +134,7 @@ impl Shape {
}
}

pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self {
pub(crate) fn change_attributes_transition(&self, key: TransitionKey, index: u32) -> Self {
match &self.inner {
Inner::Shared(shape) => Self::shared(shape.change_attributes_transition(key)),
Inner::Unique(shape) => Self::unique(shape.change_attributes_transition(key)),
Expand Down
142 changes: 83 additions & 59 deletions boa_engine/src/object/shape/shared_shape.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
use std::cell::{Cell, RefCell};
use std::{
cell::{Cell, RefCell},
collections::hash_map::RandomState,
};

use boa_gc::{Finalize, Gc, GcRefCell, Trace, WeakGc};
use indexmap::{IndexMap, IndexSet};
use rustc_hash::FxHashMap;

use crate::property::{Attribute, PropertyDescriptor, PropertyKey};
Expand All @@ -12,10 +16,7 @@ use super::{PropertyDescriptorAttribute, Shape, Slot, TransitionKey, TransitionT
struct Inner {
/// Cached property map, only constucted when a property is accesed through it.
#[unsafe_ignore_trace]
property_table: RefCell<Option<FxHashMap<PropertyKey, (u32, PropertyDescriptorAttribute)>>>,

#[unsafe_ignore_trace]
property_access: Cell<u8>,
property_table: RefCell<FxHashMap<PropertyKey, (u32, PropertyDescriptorAttribute)>>,

forward_property_transitions: GcRefCell<FxHashMap<TransitionKey, WeakGc<Inner>>>,
previous: Option<SharedShape>,
Expand All @@ -37,7 +38,7 @@ pub(crate) struct SharedShape {
}

impl SharedShape {
const DEBUG: bool = false;
const DEBUG: bool = true;

#[inline]
pub(crate) fn previous(&self) -> Option<&SharedShape> {
Expand Down Expand Up @@ -73,8 +74,7 @@ impl SharedShape {
#[inline]
pub(crate) fn root() -> Self {
Self::new(Inner {
property_table: RefCell::new(None),
property_access: Cell::new(0),
property_table: RefCell::default(),
forward_property_transitions: GcRefCell::new(FxHashMap::default()),
property_key: PropertyKey::Index(0),
property_index: 0,
Expand All @@ -98,8 +98,7 @@ impl SharedShape {
self.property_index() + 2
};
let new_inner_shape = Inner {
property_table: RefCell::new(None),
property_access: Cell::new(0),
property_table: RefCell::default(),
forward_property_transitions: GcRefCell::new(FxHashMap::default()),
property_index: next_property_index,
property_key: key.property_key.clone(),
Expand Down Expand Up @@ -148,14 +147,8 @@ impl SharedShape {
key.attributes,
);
}
let mut next_property_index = if self.previous().is_none() {
self.property_index()
} else {
self.property_index() + 2
};
let new_inner_shape = Inner {
property_table: RefCell::new(None),
property_access: Cell::new(0),
property_table: RefCell::default(),
forward_property_transitions: GcRefCell::new(FxHashMap::default()),
property_index: self.property_index(),
property_key: key.property_key.clone(),
Expand Down Expand Up @@ -200,7 +193,8 @@ impl SharedShape {
if Self::DEBUG {
println!("Shape: deleting {key}");
}
let mut transitions = Vec::default();
let mut transitions: IndexMap<PropertyKey, PropertyDescriptorAttribute, RandomState> =
IndexMap::default();
let mut p = Some(self);

let mut base = loop {
Expand All @@ -218,33 +212,46 @@ impl SharedShape {
break base;
}

transitions.push(TransitionKey {
property_key: shape.property_key().clone(),
attributes: shape.inner.property_attributes,
});
// Do not add property that we are trying to delete.
// this can happen if a configure was called after inserting it into the shape
if shape.property_key() != key {
// Only take the latest changes to a property. To try to build a smaller tree.
transitions
.entry(shape.property_key().clone())
.or_insert(shape.inner.property_attributes);
}

p = shape.previous();
};

for transition in transitions.into_iter().rev() {
for (property_key, attributes) in IntoIterator::into_iter(transitions).rev() {
let transition = TransitionKey {
property_key,
attributes,
};
base = base.insert_property_transition(transition);
}

base
}

// TODO: Implement descriptor array.
#[inline]
pub(crate) fn lookup(&self, key: &PropertyKey) -> Option<Slot> {
if Self::DEBUG {
println!("Shape: lookup {key}");
}

self.previous()?;

// Use cheched property table, if exists
if self.inner.property_access.get() == 4 {
{
let property_table = self.inner.property_table.borrow();
if let Some(property_table) = property_table.as_ref() {
if property_table.len() != 0 {
if Self::DEBUG {
println!(" Shape: Using cached property table");
}

if let Some((index, attributes)) = property_table.get(key).copied() {
return Some(Slot { index, attributes });
}
Expand All @@ -253,35 +260,44 @@ impl SharedShape {
}
}

if self.inner.property_access.get() < 4 {
self.inner
.property_access
.set(self.inner.property_access.get() + 1);

let mut p = Some(self);
while let Some(shape) = p {
// Root has no properties
if shape.is_root() {
break;
}
if shape.property_key() == key {
if Self::DEBUG {
println!(
" Shape: found {} with {} property index",
shape.property_key(),
shape.property_index()
);
}
return Some(Slot {
index: shape.property_index(),
attributes: shape.inner.property_attributes,
});
}
p = shape.previous();
}

return None;
}
// if self.inner.property_access.get() < 4 {
// self.inner
// .property_access
// .set(self.inner.property_access.get() + 1);

// let mut p = Some(self);

// let mut index = 0;
// let mut attributes = None;
// while let Some(shape) = p {
// // Root has no properties
// if shape.is_root() {
// break;
// }
// if shape.property_key() == key {
// if Self::DEBUG {
// println!(
// " Shape: found {} with {} property index",
// shape.property_key(),
// shape.property_index()
// );
// }
// if attributes.is_none() {
// attributes = Some(shape.inner.property_attributes);
// }

// if shape.transition_type() == TransitionType::Insert {
// return Some(Slot {
// index: shape.property_index(),
// attributes: attributes.expect("This should have already been set!"),
// });
// }
// }
// p = shape.previous();
// }

// return None;
// }

if Self::DEBUG {
println!(" Shape: Creating cached property table");
Expand All @@ -291,6 +307,7 @@ impl SharedShape {

// Creating cached property table
let mut result = None;
let mut attributes = None;
let mut p = Some(self);
while let Some(shape) = p {
// Root has no properties
Expand All @@ -305,16 +322,23 @@ impl SharedShape {
shape.property_index()
);
}
result = Some(Slot {
index: shape.property_index(),
attributes: shape.inner.property_attributes,
});
if attributes.is_none() {
attributes = Some(shape.inner.property_attributes);
}

if shape.transition_type() == TransitionType::Insert {
result = Some(Slot {
index: shape.property_index(),
attributes: attributes.expect("This should have already been set!"),
});
}
}

// If we already added a property we don't add the previous versions
// which can happens if a configure property transition occurs.
property_table
.entry(shape.property_key().clone())
.and_modify(|(index, _)| *index = shape.property_index())
.or_insert((shape.property_index(), shape.inner.property_attributes));

p = shape.previous();
Expand All @@ -325,7 +349,7 @@ impl SharedShape {

// Put into shape
let mut property_table_borrow = self.inner.property_table.borrow_mut();
property_table_borrow.insert(property_table);
std::mem::replace(&mut *property_table_borrow, property_table);

result
}
Expand Down

0 comments on commit 42fe677

Please sign in to comment.