feat: more efficient ownership widening

[Rich-Harris]

This is more of a placeholder than a PR — just wanted to jot my thoughts down before spending any real time on it.

#11094 caused a significant performance regression, as noted in #11117, because we do a deep read of context whenever getContext is called (so that the caller shares ownership of the context object).

I see four options:

1. Live with the performance regression (not really our style, even if it is dev-only)
2. Don't bother with ownership validation (this will result in spaghetti apps, which we very much want to avoid)
3. Don't widen ownership on getContext, and insist that people call methods on context rather than mutating it directly (personally I think this is a reasonable idea, but I was previously outvoted)
4. Make ownership widening more efficient

I like option 4. The question is whether it can be done, and I think it can. Currently, we traverse the entire object in order to add an owner to every state proxy, but I'm not sure this is necessary. What if we only traversed to the uppermost state proxy and set the owner there, and determined ownership lazily on mutation? In other words, given this...

// Parent.svelte
let stuff = $state({
  a: {
    b: {
      c: 42
    }
  }
});

setContext('blah', { stuff });

// Child.svelte
let context = getContext('blah');

...we initially set the owner of stuff to be Parent.svelte, but don't set an owner on stuff.a or stuff.a.b. Instead, we add stuff as a parent of stuff.a when we read that value, and stuff.a as a parent of stuff.a.b when we read that. (If we later do stuff.a.c = {...}, we set stuff.a as the parent of stuff.a.c, and if we do things.foo = stuff.a then things becomes an another parent of stuff.a, and so on.)

Inside getContext, we traverse the { stuff } object until we reach the stuff state proxy, and set the new owner there. (If we encountered an exotic object, i.e. a custom class instance, we would need to use the existing deep_read logic, but again only until encountering state.)

So far, the creation cost is equivalent, in that we're adding something to a set (a parent instead of an owner) every time getting a property of a state proxy results in a new state proxy (or causes a new mini-deep-read if it's an exotic object, though maybe we only need to go one level deep?). But when we widen ownership, the cost is vastly reduced, because we don't need to traverse a potentially huge object.

The cost is paid later, upon mutation, since we need to walk up the parent tree to determine ownership. But writes are much less common than reads, and they happen one at a time, so in practice the cost is greatly reduced.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: 04f57a3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[trueadm]

I think one of the concerns is maintaining the parent relationship. If you have an object with a property and assign that property to another object – so now that property exists on two objects, then thinks might get leaky and also incorrect.

[Rich-Harris]

I addressed that above — a proxy can have multiple parents:

let stuff = $state({ a: { b: { c: 42 } } });
let things = $state({});

things.foo = stuff.a; // `stuff.a` now has two parents — `stuff` and `things`

Are there cases where that would break down?

[trueadm]

@Rich-Harris it gets tricky though, right? Unless we use WeakRefs:

let stuff = $state({ a: { b: { c: 42 } } });
{
  let things = $state({});

  things.foo = stuff.a; // `stuff.a` now has two parents — `stuff` and `things`
}
// now `things` can't get GC'd as stuff.a has a strong relationship to it

[Rich-Harris]

Ah, interesting. Hadn't considered that. things would be GC'd along with stuff of course but maybe that's insufficient. Is there any particular reason not to use WeakRef? I assume there are performance implications, but at what point do they manifest — creation or dereferencing? If the latter, then maybe it's an acceptable cost.

[trueadm]

@Rich-Harris Technically, there's no guarantee they will work either – they're best avoided unless we want to add a lot of code around their management using FinalizationRegistry. They have performance costs both ways unfortunately.

[Rich-Harris]

When you say 'there's no guarantee they will work', I assume you mean one of two things:

• ref.deref() returns a value when it should return undefined
• the opposite — it returns undefined when it should return a value

Of those, the second would be a huge problem, but the first seems like it would be fine?

The perf costs can only really be assessed relative to the status quo — it seems worth trying, no?

[trueadm]

We should give it a go then!

[dummdidumm]

This doesn't solve the problem of us accidentally causing side effects by invoking getters and/or needing to traverse too much eagerly. If I have setContext{ very: { deep: { get object() { console.warn('outdated, use object2 instead'} }, get object2() { return state } } }) (with state being $state) then we still need to traverse the object until we find the state we want to widen and invoke a console.warn while doing so (which will be very confusing to the user).
And what about $state.frozen using a getter somewhere deep inside returning nested state? Same question there.
So what do we do? Do heuristics like not traversing arrays and objects with more than X keys, and not below Y levels?

Furthermore, the current ownership widening logic still has false positives as soon as you use methods to retrieve state: warning when using bind:

And why is $state({ field: 'foo' }) a warning but class Value { field = $state('foo'} is not? Isn't this equivalent?

Bottom line, the whole thing has too many holes to be a good guidance on what should or shouldn't work. I vote to remove it.

[Rich-Harris]

Furthermore, the current ownership widening logic still has false positives as soon as you use methods to retrieve state: warning when using bind:

I don't consider that a false positive — I think it should cause a warning.

And why is $state({ field: 'foo' }) a warning but class Value { field = $state('foo'} is not? Isn't this equivalent?

For the same reason that this doesn't warn — you're triggering an accessor rather than mutating a bag of state:

let field = $state('foo');
	
let no_warning = {
  get field() {
    return field;
  },
  set field(v) {
    field = v;
  }
};

Are they equivalent? You can make a case either way, but in practice if you're passing around a class instance with reactive properties it's likely that you mean for people to interact with it. The intent, as expressed through the code, is different.

Even if that feels like a gap, it's okay — as long as we avoid false positives, we're doing the right thing by steering people away from writing spaghetti code, even if we don't catch every case. Perfect is the enemy of the good here.

This doesn't solve the problem of us accidentally causing side effects by invoking getters and/or needing to traverse too much eagerly

Yeah, I don't think we should be invoking getters during getContext. A better fix for #11060 would probably be to assign ownership to the class instance's state fields directly, rather than by eagerly invoking the getters.

As a first order of business I think we should revert #11094 since it's causing perf regressions and bugs.

[trueadm]

Agreed. Let’s first revert that PR and tackle it differently.