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

Template direct field access #1227

Open
RobbinBaauw opened this issue May 24, 2020 · 17 comments
Open

Template direct field access #1227

RobbinBaauw opened this issue May 24, 2020 · 17 comments
Labels
✨ feature request New feature or request

Comments

@RobbinBaauw
Copy link
Contributor

RobbinBaauw commented May 24, 2020

What problem does this feature solve?

Currently, accessing a variable on a template makes you through several layers of proxies and getters to resolve the value you are actually looking for.

This is required because:

  1. all setup exports, props etc are access on one object, ctx, and we need to resolve the right "nested" proxy to access
  2. refs are automatically unreffed in templates, which forces a similar structure

The problem with this is that accessing many values, in a v-for for example, becomes significantly slower than direct access. In the code (https://github.com/vuejs/vue-next/blob/master/packages/runtime-core/src/componentProxy.ts#L146) some optimizations are apparent, but this still is a lot worse than direct object access.

A less important gripe is the fact that when reading this template, it is unknown whether you are dealing with a ref, a constant value, a prop etc. Edit: this could also improve IDE support as it doesn't need to search any context either, it can know the direct type.

What does the proposed API look like?

I would propose one of two solutions (though there are probably many more):

  1. Add more arguments to the created render function, which are direct references to the respective data sources. These could be prefixed with something like a $ to distinguish them from normal variables. Say you were to use $s to denote access to setupState, your template could look like {{ $s.test.value }}.

  2. Add some flag to denote if you want the backwards compatible (and maybe easier) way to use templates or this alternative, more performant one. The _ctx object could then be as such so values exported from setup can be accessed directly:

obj = {
    ...toRaw(setupState),
    props: toRaw(props),
    // etc
}

I think option 1 is more realistic, as the flag in option 2 would have to be set per component to remain backwards compatible, which would add more complexity.

To do a performance comparison, I implemented option 1 in a very naive fashion (RobbinBaauw@2ac1928), which is bound to break a few things. I have done very simple performance comparisons, between the old way, render functions & this implementation. For this I used the following component:

import { h, ref } from "vue";
import { renderList } from "@vue/runtime-core";

export default {
    setup() {
        const test = ref(0);

        setInterval(() => {
            test.value  ;
        });

        // return {
        //     test,
        // };

        return () => {
            return renderList(100_000, () => {
                return h(
                    "p",
                    {
                        "data-x": test.value,
                    },
                    "x",
                );
            });
        };
    },
};

with one of the following templates (or none, if using the render function):

<template>
    <div>
        <p v-for="n in 100000" :key="n" :data-x="$s.test.value">X</p>
    </div>
</template>

or

<template>
    <div>
        <p v-for="n in 100000" :key="n" :data-x="test">X</p>
    </div>
</template>

The performance differences were significant:

Render duration
Render function 22ms
Proxy access 61ms
Direct access 32ms

Note that I just ran the JS profiler a few times and chose average values, no extensive testing. As visible above, this was with 100k accesses to a ref which is constantly updated.

My simple implementation could probably be even more efficient, but I am not familiar enough with the codebase to give a more in depth overview about that.

What do you think about this suggestion? Does it have any merit to it?

@basvanmeurs
Copy link
Contributor

So I checked the effect of this in practice. In the case of the Vugel particles demo (https://vugel-example.planning.nl/ > particles) the time spent in renderComponentRoot is cut in half.

In praticular, there's a single line of code that costs a lot of performance right now and it's a simple reference to a setup value:
https://github.com/Planning-nl/vugel-example/blob/39c7ac3a5894fb9e554ed29d05ff1028cbe6883f/src/examples/ParticlesExample.vue#L136

I only used it once, but when used more this problem becomes more of a bottleneck as well.

Profile screenshots:
performance-profile
performance-source

@yyx990803
Copy link
Member

Yeah, the render proxy is probably the biggest bottleneck of template based rendering in Vue 3 and we are definitely open to ideas on how to circumvent that (while balancing backwards compatibility).

I like the idea of option 1. An obvious drawback is that it requires users to explicitly write code that takes advantage of it - which makes sense for perf-critical cases like Vugel, but in practice most users still wouldn't benefit from it.

Ideally, we can add an extra pass at the SFC level to analyze the returned bindings from the user component, send that info to the template compiler, and automatically transform appropriate bindings to use the "fast" access.

@yyx990803 yyx990803 added the ✨ feature request New feature or request label May 25, 2020
@RobbinBaauw
Copy link
Contributor Author

RobbinBaauw commented May 25, 2020

Of course it would be best to do this entire thing on compile time (finding the right context to access, unreffing) on compile time, but is this possible? To what extent is it possible to analyse the returned objects from the setup function / props at compile time?

Another simple but possibly too naive solution which is a derivative from option 2 would just be spreading all the contexes into the _ctx object:

_ctx = {
   ...toRaw(setupProps),
   ...toRaw(props),
   // etc
}

This would break auto unref but I don't know what else it would break.

@yyx990803
Copy link
Member

@RobbinBaauw this would require a fresh ctx object and spreading everything on every render - which comes with its own costs.

@basvanmeurs
Copy link
Contributor

When thinking of a solution, I realise that dynamic props, context and globals really complicate things. I personally think that developers shouldn't use these features in vue3, but stick to the setup vars and defined props, and use the composition api to add globals. Same goes for using $emit etc directly from a template; I prefer to wrap those in a function and export it in the setup vars. And data is also not the best way of doing things.

That said, maybe we can optimize the path for setup vars and defined props?

The idea:
For every component instance, we keep a cfns (context functions) object, which maps keys to functions that select the proper variable and performing unrefing only if necessary (what the proxy does now, but statically and fast). When applicable (it can't be used for assignments and not for v-model), a new highly-optimizable context function is created for this instance and used from that moment onwards.

My initial idea was to reuse these functions between instances, creating them only per component type, but I realised that unfortunately the returned set from setup may not always be identical. In one instance it could be a ref and in another one it could be a variable.. depending on a prop or just random behavior.

This commit is doing just that:
https://github.com/basvanmeurs/vue-next/commit/1f23842b38b57e47d55688325984b0c2f5b09095

I tested the result and it cut down the render time just as much as Robbin's commit, while using the 'normal' syntax and without breaking things. I tested heap size and it did not seem to differ, though I have to admit it didn't include a big number of components.

@basvanmeurs
Copy link
Contributor

Notice that this needs some work, as this change breaks a lot of unit tests. These failures seem to be false positives as they use a compiler snapshot as output, giving false positives. @yyx990803 are you interested in implementing this solution? Before I continue to fix the unit tests..

@basvanmeurs
Copy link
Contributor

This is a cleaner approach, using a single proxy, but optimizing that.
https://github.com/basvanmeurs/vue-next/commit/23c3a192bcaa7d4ba15fa8c8070f1f0df75d11ce

I still need to investigate the memory implications

@aztalbot
Copy link
Contributor

aztalbot commented May 27, 2020

These all seem like good solutions. Just an observation: some of the optimizations end up adding adding arguments to the render function call as well as fields to the instance. The _cache is already an existing case of doing this, since the only place to cache data in the render function is on the instance. This makes me wonder if it might make sense to add an option to the compiler to output a function that can wrap a component's setup option so that it returns the render function (example below). This way, all optimizations can occur in the setup function closure rather than tacking on caching fields to the instance or adding arguments to the render call. This would also push these constructs out of the runtime packages and purely into the compiler. I might be wrong, but I remember seeing somewhere that compiled templates are no longer supported for functional components, so I'm not sure what benefits targeting the render option vs setup would have anymore.

example:

<template>
    <div @click="inc">
      {{ category }}: {{ count }}
    </div>
</template>

becomes:

// compiler output module
import { toDisplayString, createVNode, openBlock, createBlock, withScopeId, isRef, toRaw } from "vue"

// Binding optimization for webpack code-split
const _toDisplayString = toDisplayString, _createVNode = createVNode, _openBlock = openBlock, _createBlock = createBlock, _withScopeId = withScopeId, _isRef = isRef, _toRaw = toRaw
const _withId = /*#__PURE__*/_withScopeId("scope-id")

// returns the first object that has the specified key
function coalesceByKey(key, _setup, _props) {
    let target
    if (key in _setup) {
        target = _setup
        const setupFieldValue = _setup[key]
        // can we assume that if a setup field is a ref at first, it will always be a ref?
        if (_isRef(setupFieldValue)) {
            target = {
                // yes, it has to go through getters and setters, but at least this is faster than a proxy
                get [key]() { return setupField.value },
                set [key](val) { setupField.value = val }
            }
        }
    } else if (key in _props) {
        target = _props
    }
    return target
}

// apply to any component options object to wrap its setup option
export function withSetup(_setup) {
  return (options) => {
    const scriptSetup = options.setup || (() => ({}))
    options.setup = function(...args) {
      // TODO - handle async setup and setup returning conflicting render function
      // ... also probably need to call `reactive` or a modified `toRefs` on the setup result 🤔 
      // ... also requires a way to still add this result to the component itself (to expose fields to parent when used as ref)
      return _setup(/* props */ args[0], /* setup context */ args[1], scriptSetup(...args))
    }
    return options
  }
}

export const setup = withSetup(function(_props, _sctx, _setup) {
    // below is one idea on how you could use this setup wrapper to remove proxy layers
    // pro = no need to analyze setup at compile time; con = more generated code
    // i think we can assume the keys in both props and setup will remain static, right? (or no?)
    // merging props and setup result into an object of refs might help optimize the below logic
    const inc = 'inc', count = 'count', category = 'category'
    let _inc = coalesceByKey(inc, _setup, _props), _count = coalesceByKey(count, _setup, _props), _category = coalesceByKey(category, _setup, _props)
    // using setup might(?) also mean you can hoist certain things you couldn't before
    // so long as they don't rely on $x names that can't be found in setup context (`_sctx`)
    // perhaps event handlers, slot and renderList callbacks can all be hoisted into setup in those cases
    // at the very least this closure can house `let _cache = []` or `let cfns = {}` if implementing @basvanmeurs idea
    const _setupHoisted1 = { onClick: $event => _inc[inc]($event) }
    return /*#__PURE__*/_withId(function render(_ctx) {
        _inc = _inc || _ctx, _count = _count || _ctx, _category = _category || _ctx
        return (_openBlock(), _createBlock("div", _setupHoisted1, _toDisplayString(_category[category]) + ": " + _toDisplayString(_count[count]), 1 /* TEXT */))
    })
})

Implementing this would take me quite some time, so please forgive me for not having an implementation branch. I've tested a version of the above manually and it seems to work fine, but I'm sure there are issues with it as a solution. Mainly, I found it interesting to explore how targetting setup could provide more flexibility in how you approach these kinds of optimizations, so I thought I'd share.

@basvanmeurs
Copy link
Contributor

basvanmeurs commented May 29, 2020

// i think we can assume the keys in both props and setup will remain static, right? (or no?)

Unfortunately, no. Every instance's setup, even of the same component type, may return a different object with different keys. So for instance A, the key 'x' may refer to a setup var, while for instance B it might refer to a prop. This complicates things, unfortunately.

@aztalbot
Copy link
Contributor

aztalbot commented May 29, 2020

@basvanmeurs I meant in the context of a single instance, since setup will run once per instance and the code in that example is only trying to determine where to find that value for the lifetime of that specific instance. What I was going for is that we know all the keys we need to reference based on the template, so we can wrap the render function in setup to find the most immediate objects on which to access each field (we could even create a single object with immediate access to the values of each key) during all subsequent renders for that specific instance. If a key is not present during setup, but is added later, we would fallback to finding it on the context proxy. So mainly I was hoping we can assume that an instance will never move a key from, say, setup to $data after some update during it own lifetime (can't think of why that would ever happen, but wasn't sure if that's a thing that needs to be supported).

@RobbinBaauw
Copy link
Contributor Author

RobbinBaauw commented Jul 3, 2020

Other advantages of direct field access

@yyx990803 I would also like to provide some alternative arguments than performance. Note that these are my personal opinions :)

First and foremost (arguably even more important than performance), readability. When working in teams, many pieces of code will not be written by you, but you are expected to fix bugs. Right now, it is hard to determine where a field comes from, it can be a prop, something from data or maybe something returned by the setup function. I believe it would improve readability by a lot, as you instantly see the context and don't have to look for the place where it is defined.

Another argument is that you can currently get naming conflicts. Sure, there can be a warning on dev mode (not sure if there is one), but there are plenty of valid scenarios in which both a prop and value returned from the setup function may have the same name, which is currently not possible, since all is defined in the same namespace.

Then, I think auto-unreffing is unintuitive. Vue 3 is built around the idea of writing "correct" JS and having an explicit .value instead of some hacks (which is great!), but in the templates, we drop this .value. I understand that this makes it so that you have to type a few less characters, but I like explicitness and consistency over these few characters, especially since it makes it clear that it is a reactive value instead of some static value.

Finally, there are no proxies which you need to take into account. See #1499.

My suggestion

So: I still stand by the suggestion of using something like $p, $s etc. (there are probably much better semantics, so this could definitely change) to allow for direct field access. On compile-time it can be determined whether we should add _ctx or not, depending on the prefix of the value. For users that don't want to type these extra characters, backwards-compatibility or to provide an easier learning curve, nothing would change. For users that want more explicitness, readability or performance (which I believe is the same group anyway), they could use direct field access.

I'd be happy to open an RFC for this.

@yyx990803
Copy link
Member

yyx990803 commented Jul 3, 2020

You already can use explicit $data and $props in templates, it's just the overhead of getting them off the _ctx proxy that needs to be removed.

Note that I'm not against the overall idea at all. It's something we will definitely implement.

What I think we should do:

  1. Add $setup for direct access to bindings returned from setup, and $ctx for everything else (methods/computed/inject declared via options API)

  2. Pass them into the render function as arguments so that we no longer need to go through _ctx to access them. The compiler will special case these keys and avoid prefixing them.

  3. Make the template compiler accept additional metadata about binding information, e.g.:

    compile(`<div>{{ foo }}</div>`, {
      bindingMetadata: {
        foo: BindingTypes.DATA
      }
    })
  4. When transforming the expressions, use bindingMetadata to determine the output. For example, currently foo is transformed into _ctx.foo, but with the metadata it is now $data.foo

  5. An additional step in SFCs that analyzes the <script> part to generate the bindingMetadata. Note the analysis doesn't need to offer 100% coverage because anything that isn't specified in bindingMetadata can just fallback to _ctx for runtime dispatching. But for those that can be analyzed at compile time, they will be able to get direct access.

  6. The analyze step can also be used by external tooling like Vetur and eslint-plugin-vue for IDE / template lint support.

@yyx990803
Copy link
Member

Update: still WIP but the idea is working (screenshot of actual working code running in Vite):

Screen Shot 2020-07-10 at 10 19 02 PM

@RobbinBaauw
Copy link
Contributor Author

Great! I was just thinking of something else: why is the setupState even reactive? I understand that props need to be reactive (although they are only shallowReactive), but I don't see the case where you should be able to reactively add new properties to this setupState object.

Couldn't you make it a normal object and just handle the auto unreffing in the component proxy, in the renderer in setRefs and possibly other cases? You'd lose the recursive reactivity but I don't think people would even expect that to happen: if I were a new user I would be searching for ages as to why the properties returned by my plain setup object I returned are suddenly behaving differently (because of this recursive reactivity).

This would reduce the amount of layers of proxies a lot as well, increasing performance all across the board. If this is not possible / logical, making it a shallowReactive like props makes more sense to me as well.

Both using a plain object and shallowReactive (though a plain object is of course more performant) solve the "issue" I described in #1499, as I still believe it's very weird behavior for an end user.

@jods4
Copy link
Contributor

jods4 commented Jul 11, 2020

@RobbinBaauw If you dig through issues, you'll find one I opened during early betas to suggest not using any proxy at all on the returned state from setup.

This was done and then reversed. As I recall, the main argument was that Vue wants to unref proxies automatically in templates, and deeply so.

@RobbinBaauw
Copy link
Contributor Author

RobbinBaauw commented Jul 11, 2020

Hm okay, I disagree with that sentiment as said above but that sounds like a similar proposal. I think 1 layer of auto unref would be perfect (and doable without proxies), and I can envision a multitude of "layer 8" issues related to the current behavior popping up once the masses start to use Vue 3 😄 (just as mine, even though I knew of the proxies)

I also think it'd be possible to drop the entire component proxy as well, though that requires a bit more compiler work which is probably not worth it as it's not that big of a performance bottleneck once the change Evan is currently working on is done.

Edit: I think only auto unreffing the last layer would work for most cases. Even some deep access like

class C {
    d = ref(1)
}

class A {
    b = ref(new C())
}

return {
    a: ref(new A())
}

with {{ a.b.c }} would work because the ref(new A()) instead of a shallowRef. So instead of Vue deciding for the user that everything must be a proxy, the user can decide it themselves. The only difference with now is that you must make new A() a ref, instead of it being automagically reactive, but I believe this explicitness is much, much better.

@basvanmeurs
Copy link
Contributor

basvanmeurs commented Jul 14, 2020

First of all, I am completely with @RobbinBaauw on this one. I'd like to see the reactive setupState being changed to shallowReactive.

The recursive feature of the setup state's reactive proxy just feels like it's doing more than it should. Really, why are my instances turning into proxies by just returning them in setup state?

And it has undesired and unexpected implications on the runtime - as shown by Robbin.

But, a more serious implication I doubt the core team is aware of is shown in this Codepen.
https://codepen.io/basvanmeurs/pen/MWKqejd

In short: just run any instance of any class once with a reactive proxy and it will destroy performance for all instances of that class!

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

No branches or pull requests

5 participants