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

[mansiverma897993]

This Pull Request fixes/closes #5399.

It changes the following:

• Replaces GcRefCell<ArrayVec<CacheEntry, PIC_CAPACITY>> with Cell<ArrayVec<CacheEntry, PIC_CAPACITY>> in InlineCache structure.
• Uses Cell::take() and Cell::set() to access and modify the inline cache entries in a zero-overhead manner, avoiding all dynamic borrow-checking overhead on the hot path.
• Manually implements Clone and Debug for InlineCache because Cell<T> only auto-derives them when T: Copy, and ArrayVec is non-Copy.
• Replaces .entries.borrow() with a new test helper .entries() in core/engine/src/vm/inline_cache/tests.rs to allow unit testing without borrow checks.

[github-actions]

Test262 conformance changes

Test result	main count	PR count	difference
Total	53,125	53,125	0
Passed	51,072	51,072	0
Ignored	1,482	1,482	0
Failed	571	571	0
Panics	0	0	0
Conformance	96.14%	96.14%	0.00%

Tested main commit: 8a1e8fe07f626f7a067afc2c9885d5d87de4bb5d
Tested PR commit: 6da74d42e8d408a8123f8fa9b5e8a093a25af456
Compare commits: 8a1e8fe...6da74d4

[jasonwilliams]

Thanks for this @mansiverma897993, you need to run rust fmt

[jasonwilliams]

For the clone it feels inefficient to take the value out, put a default one in then add the value back on a hotpath. We may need to get a pointer into the cell here as its safe (we are taking and resetting in the same operation). Something like this:

    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 {
            name: self.name.clone(),
            entries: Cell::new(cloned_entries),
            megamorphic: self.megamorphic.clone(),
        }
    }

Also, while you're here, can you move the megmorphic to be the first item in the struct? The entries value is so big it pushes the bool out of the cacheline so its a cache miss every time it's read, it may need the #[repr(C)] tag above

[mansiverma897993]

@jasonwilliams I have sucessfully update the PR as per your accordance !! look at once

[codecov]

Codecov Report

❌ Patch coverage is 42.10526% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.30%. Comparing base (6ddc2b4) to head (6da74d4).
⚠️ Report is 985 commits behind head on main.

Files with missing lines	Patch %	Lines
core/engine/src/vm/inline_cache/mod.rs	42.10%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5400       +/-   ##
===========================================
+ Coverage   47.24%   60.30%   +13.05%     
===========================================
  Files         476      567       +91     
  Lines       46892    63162    +16270     
===========================================
+ Hits        22154    38089    +15935     
- Misses      24738    25073      +335     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasonwilliams]

@mansiverma897993 thanks, I'm not sure when I can take a look again. In the meantime are you able to compare the benchmarks between main and this? We have a data repo here: https://github.com/boa-dev/data

You can check that out in an adjacent folder and run boa against the bench/bench-v8/combined.js file

When I checked on cachegrind it looked like there were still a high amount of cache misses happening somewhere in that function, maybe @HalidOdat is better at hunting these things down.

[zhuzhu81998]

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]

"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]

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