Skip to content

perf: remove GcRefCell from inline cache to avoid borrow checking overhead #5400

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.

Sign up for GitHub

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
mansiverma897993 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

perf: remove GcRefCell from inline cache to avoid borrow checking overhead #5400

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2d4cd2
perf: remove GcRefCell from inline cache to avoid borrow checking ove…
mansiverma897993 Jun 17, 2026
8c31742
style: format inline_cache/mod.rs
mansiverma897993 Jun 18, 2026
bbe3e96
perf: optimize inline cache cloning and struct layout
mansiverma897993 Jun 18, 2026
6da74d4
perf: avoid copying inline cache entries by using raw pointer access
mansiverma897993 Jun 28, 2026
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
69 changes: 56 additions & 13 deletions core/engine/src/vm/inline_cache/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use arrayvec::ArrayVec;
use itertools::Itertools;
use std::{cell::Cell, fmt};

use boa_gc::GcRefCell;
use boa_macros::{Finalize, Trace};

use crate::{
Expand All @@ -25,17 +24,48 @@ pub(crate) struct CacheEntry {
}

/// An inline cache entry for a property access.
#[derive(Clone, Debug, Trace, Finalize)]
#[repr(C)]
#[derive(Trace, Finalize)]
pub(crate) struct InlineCache {
/// Whether this access site has seen too many shapes and should no longer be cached.
#[unsafe_ignore_trace]
pub(crate) megamorphic: Cell<bool>,

/// The property that is accessed.
pub(crate) name: JsString,

/// Multiple cached shape-to-slot entries.
pub(crate) entries: GcRefCell<ArrayVec<CacheEntry, PIC_CAPACITY>>,
pub(crate) entries: Cell<ArrayVec<CacheEntry, PIC_CAPACITY>>,
}

/// Whether this access site has seen too many shapes and should no longer be cached.
#[unsafe_ignore_trace]
pub(crate) megamorphic: Cell<bool>,
impl Clone for InlineCache {
fn clone(&self) -> Self {
// SAFETY: `entries` is only ever accessed through `&self`/`&mut self`
// on this single-threaded cache, and cloning `CacheEntry` doesn't
// reenter this `Cell`, so it's safe to read through the raw pointer
// for the duration of this borrow without disturbing the cell's contents.
let cloned_entries = unsafe { (*self.entries.as_ptr()).clone() };

Self {
megamorphic: self.megamorphic.clone(),
name: self.name.clone(),
entries: Cell::new(cloned_entries),
}
}
}

impl fmt::Debug for InlineCache {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let entries = self.entries.take();
let result = f
.debug_struct("InlineCache")
.field("name", &self.name)
.field("entries", &entries)
.field("megamorphic", &self.megamorphic)
.finish();
self.entries.set(entries);
result
}
}

impl fmt::Display for InlineCache {
Expand All @@ -46,28 +76,38 @@ impl fmt::Display for InlineCache {
return write!(f, "(megamorphic))");
}

let entries = self.entries.borrow();
let entries = entries.iter().map(|e| e.shape.to_addr_usize()).format(", ");
let entries = self.entries.take();
let formatted = entries.iter().map(|e| e.shape.to_addr_usize()).format(", ");
let result = write!(f, "({formatted:#x}))");
self.entries.set(entries);

write!(f, "({entries:#x}))")
result
}
}

impl InlineCache {
pub(crate) fn new(name: JsString) -> Self {
Self {
name,
entries: GcRefCell::new(ArrayVec::new()),
megamorphic: Cell::new(false),
name,
entries: Cell::new(ArrayVec::new()),
}
}

#[cfg(test)]
pub(crate) fn entries(&self) -> ArrayVec<CacheEntry, PIC_CAPACITY> {
let entries = self.entries.take();
let cloned = entries.clone();
self.entries.set(entries);
cloned
}

pub(crate) fn set(&self, shape: &Shape, slot: Slot) {
if self.megamorphic.get() {
return;
}

let mut entries = self.entries.borrow_mut();
let mut entries = self.entries.take();

// Add a new entry if there's space.
if entries
Expand All @@ -81,6 +121,8 @@ impl InlineCache {
self.megamorphic.set(true);
entries.clear();
}

self.entries.set(entries);
}

/// Returns the cached `(Shape, Slot)` if a matching shape exists in the inline cache.
Expand All @@ -91,7 +133,7 @@ impl InlineCache {
return None;
}

let mut entries = self.entries.borrow_mut();
let mut entries = self.entries.take();

@zhuzhu81998 zhuzhu81998 Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take on Cell performs a copy i think. perhaps its not trivial to copy the entire cache twice (once here once below for setting) everytime you try to access or set it?
Given how often this is used.

@mansiverma897993 mansiverma897993 Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Thanks for pointing this out! You are absolutely right, Cell::take() does copy the entire ArrayVec which adds overhead on the hot path.

To avoid this copy while still avoiding GcRefCell's borrow checking, we can use Cell::as_ptr() to mutate the entries in-place using a raw pointer:

rust
let entries = unsafe { &mut *self.entries.as_ptr() };
Since this is single-threaded and the reference doesn't escape the function, this should be safe and provide zero-copy access. I'll update the PR with this change!" I am updating PR accordingly I am working on it !!

@mansiverma897993 mansiverma897993 Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhuzhu81998 @jasonwilliams I have updated PR accordingly now look at once!!

let mut i = 0;
let mut result = None;
let shape_addr = shape.to_addr_usize();
Expand All @@ -109,6 +151,7 @@ impl InlineCache {
}
}

self.entries.set(entries);
result
}
}
20 changes: 10 additions & 10 deletions core/engine/src/vm/inline_cache/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ fn set_property_by_name_set_inline_cache_on_property_load() -> JsResult<()> {
let (function, code) = get_codeblock(&function).unwrap();

assert_eq!(code.ic.len(), 1);
assert_eq!(code.ic[0].entries.borrow().len(), 0);
assert_eq!(code.ic[0].entries().len(), 0);

let o = ObjectInitializer::new(context)
.property(js_string!("test"), 0, Attribute::all())
Expand All @@ -339,9 +339,9 @@ fn set_property_by_name_set_inline_cache_on_property_load() -> JsResult<()> {

function.call(&JsValue::undefined(), &[o.clone().into()], context)?;

assert_eq!(code.ic[0].entries.borrow().len(), 1);
assert_eq!(code.ic[0].entries().len(), 1);
assert_eq!(
code.ic[0].entries.borrow()[0]
code.ic[0].entries()[0]
.shape
.upgrade()
.unwrap()
Expand All @@ -359,7 +359,7 @@ fn get_property_by_name_set_inline_cache_on_property_load() -> JsResult<()> {
let (function, code) = get_codeblock(&function).unwrap();

assert_eq!(code.ic.len(), 1);
assert_eq!(code.ic[0].entries.borrow().len(), 0);
assert_eq!(code.ic[0].entries().len(), 0);

let o = ObjectInitializer::new(context)
.property(js_string!("test"), 0, Attribute::all())
Expand All @@ -368,9 +368,9 @@ fn get_property_by_name_set_inline_cache_on_property_load() -> JsResult<()> {

function.call(&JsValue::undefined(), &[o.clone().into()], context)?;

assert_eq!(code.ic[0].entries.borrow().len(), 1);
assert_eq!(code.ic[0].entries().len(), 1);
assert_eq!(
code.ic[0].entries.borrow()[0]
code.ic[0].entries()[0]
.shape
.upgrade()
.unwrap()
Expand All @@ -388,7 +388,7 @@ fn test_polymorphic_inline_cache() -> JsResult<()> {
let (function, code) = get_codeblock(&function).unwrap();

assert_eq!(code.ic.len(), 1);
assert_eq!(code.ic[0].entries.borrow().len(), 0);
assert_eq!(code.ic[0].entries().len(), 0);
assert!(!code.ic[0].megamorphic.get());

let shapes = vec![
Expand All @@ -413,7 +413,7 @@ fn test_polymorphic_inline_cache() -> JsResult<()> {
function.call(&JsValue::undefined(), &[o.clone().into()], context)?;
}

assert_eq!(code.ic[0].entries.borrow().len(), 4);
assert_eq!(code.ic[0].entries().len(), 4);
assert!(!code.ic[0].megamorphic.get());

Ok(())
Expand Down Expand Up @@ -451,7 +451,7 @@ fn test_megamorphic_inline_cache() -> JsResult<()> {
function.call(&JsValue::undefined(), &[o.clone().into()], context)?;
}

assert_eq!(code.ic[0].entries.borrow().len(), 0);
assert_eq!(code.ic[0].entries().len(), 0);
assert!(code.ic[0].megamorphic.get());

// Regression check: repeated miss should remain empty
Expand All @@ -460,7 +460,7 @@ fn test_megamorphic_inline_cache() -> JsResult<()> {
.property(js_string!("test"), 1, Attribute::all())
.build();
function.call(&JsValue::undefined(), &[o6.clone().into()], context)?;
assert_eq!(code.ic[0].entries.borrow().len(), 0);
assert_eq!(code.ic[0].entries().len(), 0);
assert!(code.ic[0].megamorphic.get());

Ok(())
Expand Down