[Perf] - Consider not GCing function environments which don’t need it

[jasonwilliams]

Describe the bug
I did some profiling on one of the benchmarks Richards. I don’t have the profile data to hand right now but around 20% of time was spent making function environments. A function environment is created on every function call.

The reason for the slow down is because every new function environment is wrapped in GC::New(), most of the GC congestion is coming from creating function environments. The GC eventually needs to collect, so has to stop the world and collect very often.

The benchmarks accentuate the problem because many functions are being called in a loop, so push_function is being called a lot.

Potential Solution

My question is, do we need to wrap all function environments in a GC? We need to keep them around if there’s re-entry (such as async or generators) or if we have a closure escape, but if a function has neither of these we could have a “fast path” where we bypass the GC wrapping.

We already perform escape analysis on functions at the AST level, but we don’t keep the result.

We could have some “is_reentry” field that is passed down into the code block to tell us if the function has any escape or any reentry. This boolean would be sent into push_function here

Then we can set how we want to store the environment inside of push_function This would require a refactor where we have an enum which represents an environment on the stack or in a GC:

#[derive(Clone, Trace, Finalize)]
pub enum EnvironmentPointer {
 NonGcAllocated(Box<DeclarativeEnvironment>),
 GcAllocated(Gc<DeclarativeEnvironment>),
}

Then used in the outer Environment enum as:

#[derive(Clone, Trace, Finalize)]
pub enum Environment {
    Declarative(EnvironmentPointer),
    Object(Gc<ObjectEnvironment>)
}

To Reproduce

1. Checkout the boa/data repo. You can do a partial checkout of just the bench/bench-v8 subdirectory.
2. build Boa with the release-dbg profile.
3. in the Makefile from the data repo remove the tests but keep Richard’s, run Make
4. Use flamegraph or valgrind and run Boa against combined.js

Expected behavior
We see less time spent in push_function an GC Contention.

[nekevss]

Huh, I'd be curious about the numbers on the Box<DeclarativeEnvironment>. Most Gc::new calls should be a Box::new and potentially collection under the hood.

So we could increase the threshold at which the GC collects if we are triggering a collection that often. We could also look into switching to a different allocator with a ready implemented Box.

[jasonwilliams]

Huh, I'd be curious about the numbers on the Box<DeclarativeEnvironment>. Most Gc::new calls should be a Box::new and potentially collection under the hood.

Yeah I think the actual allocation is similar to Box, so we probably wouldn’t see much difference here. The issue is the collection. Because every environment is going to the GC we’re collecting a lot more than expected and this is where the slow down is.

So we could increase the threshold at which the GC collects if we are triggering a collection that often. We could also look into switching to a different allocator with a ready implemented Box.

I remember looking at the thresholds before but we could take a second look. Are you talking about a different allocator across the board or just for function environments?

[nekevss]

Are you talking about a different allocator across the board or just for function environments?

Different allocator across the board. Granted, this is getting more into the territory of the rewrite, but I'd be curious if there were allocators with an already available box (like bumpalo) that could be dropped in and work.

[jasonwilliams]

Last time I bumped it to 1MB 38d0324, it may be too low but would need some measuring to see what it should be adjusted to. I do wonder if the original idea of avoiding the GC path completely if it’s not needed is worth doing though.

[nekevss]

Hmmmm, if it's triggering a collection that often and doesn't need to be, then yeah, it might be beneficial just boxing it.

[HalidOdat]

off-topic: Hmmm... Ideally in most cases we shouldn't need to emit a function scope, will try to eliminate some of them, it should give us a big perf boost 😄

EDIT: #4266

[HalidOdat]

Checked the benchmark functions with #4266 and only a handful of functions allocate function scopes now :)