Skip to content
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.

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

Fix helper/modifier memory leak #1440

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

bendemboski
Copy link
Contributor

This is a stab at addressing emberjs/ember.js#20535

I'm very unfamiliar with this code, so this may be more of a start-a-discussion pull request rather than the actual solution. The commit message describes the fix. A few issues/questions are laid out below.

Overall approach

The goal here is to make sure the runtime doesn't keep strong references to definitions that are meant to have more limited lifetimes, such as helpers/modifiers that are "owned by" or "bound to" an instance of a component, e.g. in the case of a component class field containing a helper/modifier definition. I'm not sure this perfectly captures that distinction, but I think it gets "close enough"?

If I understand the code correctly, what I've done here is made a distinction between helpers/modifiers resolved by the opcode compiler (that requires a handle rather than a raw definition) and ones resolved dynamically by the runtime (in which case a handle is not needed, just a raw definition). This seems correct?

Caching & performance

In addition to doing the handle/definition tracking, ConstantsImpl also has a caching layer, so this removes that caching for dynamic helper/modifier definitions. I'm not sure if this is acceptable from a performance standpoint, but I hope it is! If not, I imagine we could add caches for dynamic helper/modifier definitions to somewhere in the VM without too much effort.

Components

ConstantsImpl also holds strong references to dynamic components, but .component() returns a ComponentDefinition which has a handle field, so divorcing this from ConstantsImpl and its strong references seems like a much bigger task. Perhaps that task is worthwhile, but given that (I'm pretty sure) in Ember local helpers and modifiers are much more common than local components, it seems worthwhile to plug the helpers/modifiers leak sooner, and separately assess the value/effort/feasibility of addressing the dynamic components leak.

The screwy code

The code for resolving the dynamic helpers/modifiers ended up slightly screwy because it was collapsing two layers of functions with debug asserts and type narrowing and whatnot, and I wanted to leave the unit tests untouched (even the ones testing for specific assertions) just to make it very clear that, as far as I can tell, this change has no effect on the VM other than the caching. I'm more than happy to go and clean up that code a bit and adjust the unit tests accordingly if anyone thinks it's worthwhile.

New unit tests

I didn't add any new unit tests. This PR produces no "functional" change, so the only test I can think of would be to use WeakRefs (and window.gc() along with the --expose-gc flag) to show that dynamic helpers/components aren't retained by the VM. I'm happy to do this if we think this PR is going in the right direction.

@NullVoxPopuli
Copy link
Contributor

I do think we need a way to assert that we don't have memory leaks -- but hopefully that can be applied across the whole test suite, rather than a specific test?

@bendemboski
Copy link
Contributor Author

@NullVoxPopuli that would be nice, but the problem is that a "leak" isn't a super well-defined concept at this level. The issue this PR is addressing is a case-in-point -- under some usage patterns, ConstantsImpl retaining some objects for its entire lifetime is not a leak, while under other usage patterns, ConstantsImpl retaining some objects for its entire lifetime is a leak. So I'm not sure how we do this kind of thing across the entire test suite.

What we can in theory test for across the entire test suite is that once each test has completed, certain objects that we've identified (somehow) are no longer in memory, which assumes that:

  1. We can identify those objects in a test-detectable way
  2. Each test does all the teardown it should to ensure that those objects should have been released

I have done something like that in my test suite and it's helpful for detecting the kinds of leaks where after tearing down the test container, things are still in memory. But it didn't catch this "leak" because after the test container is torn down, I guess the VM is torn down and all the modifiers/helpers are released. This is a "leak" because while the VM is running (i.e. while the Ember app is running) it holds on to every dynamic helper/modifier definition instance, so repeatedly rendering and un-rendering components that call such helpers/modifiers causes memory to grow unboundedly, as long as the app/VM is running.

It's not clear to me that any kinds of across-the-board test-suite-level instrumentation could possibly catch things like this.

@acorncom acorncom requested a review from wycats September 2, 2023 12:08
@bendemboski
Copy link
Contributor Author

@chancancode I don't think we're waiting from anything from me on this, right? What needs to happen to move it forward?

@chancancode
Copy link
Contributor

Probably @NullVoxPopuli and @wycats ?

@chancancode
Copy link
Contributor

chancancode commented Sep 25, 2023

I think I already said this somewhere else: as far as I can tell, it seems good. It doesn't not make sense in the "values mode" to add things to the constant pools:

{{foo "bar"}} <- "resolution mode", constant
{{helper "foo" "bar"}} <- "resolution mode", constant
{{helper this.foo "bar"}} <- if `this.foo` is a string, then it's resolution mode and the result of the resolution is constant for the given name
{{helper this.fooFunction "bar"}} <- if `this.foo` is a helper/function, then it's value mode and is not constant
import foo from "foo";

<template>
  {{foo "bar"}} // <- value mode
  {{helper foo "bar"}} // <- value mode
</template>

class Foo {
  <template>
    {{this.foo}} // <- value mode
  </template>
  
  foo = () => "foo";
}

@NullVoxPopuli
Copy link
Contributor

I think the changes are good, too.
I've been lagging behind on merging stuff is glimmer-vm, because I need to make sure ember-source is up to date with glimmer-vm.

I think at this point though, we should just merge (esp as the test suite is passing).
ember-source can upgrade glimmer at any time
(though, we do need to publish a new version)

@NullVoxPopuli
Copy link
Contributor

sorry I'm a bit behind, just started trying to get ember-source updated: emberjs/ember.js#20561

once that's done, I can try to test this more thoroughly

@bendemboski
Copy link
Contributor Author

@NullVoxPopuli any chance we can get this moving again?

@rlivsey
Copy link

rlivsey commented Dec 13, 2023

We've used patch-package to bring this fix into production at Intercom to fix some memory leaks we'd seen, and seems to have worked well.

@NullVoxPopuli
Copy link
Contributor

@bendemboski ye, can you rebase?

By treating passing dynamic helpers and modifiers through the `ConstantsImpl`, the runtime was ensuring that they would be retained for the lifetime of the runtime. However, dynamic helpers and modifiers are often mean to have much shorter lifetimes. For example, in the case of local helpers/modifiers inside Component classes, they are often expected to be released when the component is destroyed. If the local helper/modifier is implemented by an arrow function that closes over the Component, then retaining the arrow function will also retain the component, causing the component to leak.

So, skip `ConstantsImpl` and resolve the dynamic helper/modifier directly. Note that this means these dynamic helper/modifier definitions will no longer be cache, and the dynamic helper/modifier manager lookup and invocation will happen each time the helper/modifier is resolved.
@bendemboski
Copy link
Contributor Author

@NullVoxPopuli done.

We have also had this in production via patch-package for a couple months and have seen only desirable results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants