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

Unnecessary rendering updates when props change #1790

Closed
skirtles-code opened this issue Aug 5, 2020 · 12 comments
Closed

Unnecessary rendering updates when props change #1790

skirtles-code opened this issue Aug 5, 2020 · 12 comments

Comments

@skirtles-code
Copy link
Contributor

Version

3.0.0-rc.5

Reproduction link

https://jsfiddle.net/skirtle/oxfbL72k/3/

Steps to reproduce

  1. Open the console log.
  2. Click the button.
  3. Observe that the child component's render function has been called.

What is expected?

The render function should not be called.

What is actually happening?

The render function is being called.


The changed prop is not used by the render function so rendering should not be triggered.

There are several use cases for props that don't require rendering updates. e.g.:

  1. The prop could be an id used to load data behind the scenes.
  2. The prop might be used within an event handler, so the value isn't read until the event occurs.

The actual performance impact will usually be small as no DOM changes should be required. It could add up though if lots of components update at once, maybe inside a list or grid component.

@posva
Copy link
Member

posva commented Aug 5, 2020

The actual performance impact will usually be small as no DOM changes should be required. It could add up though if lots of components update at once, maybe inside a list or grid component.

Have you done a benchmark to see the difference between <child-component/> (no prop) and <child-component :value="id"/>?

@qgates
Copy link

qgates commented Aug 6, 2020

@posva sorry to chime in but I think the OP's point is that the render function could involve more significant changes than in the demo. I'm curious as to how optimised things are underneath ie. the render function is called but vue doesn't actually do anything since the template doesn't need updating.

Is a call to the render function when props change by design and the render itself optimised by vue internally?

@underfin
Copy link
Member

underfin commented Aug 6, 2020

See #1181 (comment)

@posva
Copy link
Member

posva commented Aug 6, 2020

@underfin thanks, I knew we had the explanation of correctness somewhere

@posva posva closed this as completed Aug 6, 2020
@qgates
Copy link

qgates commented Aug 6, 2020

@underfin thanks for clarifying. Seems a behavioural change from vue2, is it mentioned in the docs?

https://jsfiddle.net/e3t7sby2/

@skirtles-code
Copy link
Contributor Author

@posva @underfin

Much of the discussion on #1181 seems to be about $attrs, style and class.

The discussion of props starts with this comment from Evan:

Why would a component declare a prop and then never use it?

I can think of several use cases for props that aren't used during rendering:

  1. The prop could be an id used to load data behind the scenes.
  2. The prop might be used within an event handler, so the value isn't read until the event occurs.
  3. The prop may be used conditionally and the relevant condition isn't currently met. e.g. an error message only shown when an input fails validation. If the locale changes then all the error message props would change but no actual rendering needs to occur unless one of those errors is visible.

Evan also comments:

When you work on a framework you need to understand the trade-offs: premature optimizations often end up in gains that are simply too small to justify the extra complexity and edge cases they create.

I wholeheartedly agree. Perhaps avoiding the extra rendering isn't worth it in this case. I'm unclear precisely what changes would be required to check which props are used during rendering, so I'm not in a position to evaluate the trade-offs. However, that evaluation needs to be made with the understanding that use cases for 'unused' props do exist in real applications.

@jods4
Copy link
Contributor

jods4 commented Sep 6, 2020

I read @yyx990803 comments in #1181, but I just don't get it.

He says correctness is more important than optimizations and I wholly agree with that.
But what is the correctness issue here?

If a specific prop is not a dependency of rendering (there can be many good reasons for that), I don't see what issue could arise in not rendering again when said prop changes.
Isn't it akin to say: all your watches should run again when a prop changes, even if they don't read that prop!?

It looks like added complexity to me: somehow those unread dependencies (props) must be added to the effect, somehow.
And it has further implications than just runtime performance:

  • extra code implies maintenance, possibly bugs, bigger code size, etc.
  • once this behavior is published, some people will depend on it and it will not be possible to remove it.
  • other people have noted this is actually a breaking change from v2 in Further RFCs prior to 3.0.0 release rfcs#209, which in itself is a reason not to do it unless there is a stronger motivation.

@jods4
Copy link
Contributor

jods4 commented Sep 6, 2020

I'm gonna add one more reason to be very careful here!
In an ideal world, everything is pure (without side-effects), especially render functions.
Reality isn't so and look no further than core Vue itself for an example:
Layout animations are typically done by looking at changes across renders and trigger the required animations, which is a side-effect.
This process could be thrown off by unexpected no-op renders.
What makes it worse in this case is that the bug would be spread across different codebases, one part (the side-effect) being in a 3rd party library (providing the <Transition>) and the other part (the changing prop) being in a specific piece of use code. That's amongst the worse kind of bugs to fix.
--> Predictable renders is a nice feature.

@CyberAP
Copy link
Contributor

CyberAP commented Sep 6, 2020

This change actually introduces a lot of pain when working with renderless provider components which are still viable in Vue 3. Provider components generally wrap your whole app and do not render anything other than the default slot. If I understand this correctly updating provider would cause a whole app VNode tree to re-render.

<ProviderFoo>
  <ProviderBar>
    <App />
  </ProviderBar>
</ProviderFoo>
export default {
  name: 'App',
  inject: ['providedByFoo'],
  mounted() {
    this.providedByFoo.value = 1;
    // whole app re-rendered
  },
}

@skirtles-code
Copy link
Contributor Author

@CyberAP I don't think your example would be affected as it doesn't include any props.

@jods4 I did a bit of digging to try to understand this better. Here is my (very rough) explanation of what is going on.

There are two ways that a component update (re-render) can be triggered in Vue 3.

Firstly, if any reactive rendering dependencies change then the component will be added to the scheduler queue. This covers cases such as changes to data.

However, changes to props are not handled that way. They are handled by the other update path.

The only thing that can change props is a parent update. Rather than going through the scheduler queue the parent triggers child updates itself.

The decision about whether or not to update the child is made in shouldUpdateComponent, code here:

https://github.com/vuejs/vue-next/blob/d4724619fc3d005311f27c1ac7cab0a0e735c4d2/packages/runtime-core/src/componentRenderUtils.ts#L288

That checks whether the 'props' have changed but doesn't take into account how they are used.

Note that the 'props' here are not props in the usual sense. These are the raw VNode props, so they also include $attrs and listeners. Partitioning and normalization based on the props and emits configurations occurs at a later stage and isn't taken into account at this stage. It is just comparing the raw VNode attributes.

This means that component prop changes are not the only thing that can trigger an unnecessary render. Some examples:

  1. The name normalization, defaults and special handling for type: Boolean are not yet applied. Any one of these can cause changed raw props to be translated into unchanged component props. But the rendering update is based on the raw props.
  2. Inline listeners. e.g. @toggle="handler(item)". Adding toggle to emits will remove it from $attrs but that won't help. Every time the parent renders it will create a new wrapper function for the listener and that will count as a change, so the child will re-render.

@jods4
Copy link
Contributor

jods4 commented Sep 7, 2020

@skirtles-code Thanks for doing the digging, I hadn't looked deeply into these parts yet.

So as I understand your findings, it's not a correctness issue but rather ease of implementation:
that second direct update path would have a hard time determining if a change in the parent is or is not amongst the dependencies of the render effect.

That is understandable.

I'll dig it myself when I have time, but do you happen to know how that second update path avoids double rendering?
If a used property changes, from what you explained I assume there would be a direct rendering, but the rendering effect would also trigger, wouldn't it? What's in place to avoid rendering twice?

@nandin-borjigin
Copy link

nandin-borjigin commented Jan 9, 2021

@posva Please tell me if I should create a new issue instead of commenting here.


I'm facing same problem and wondering if this usecase justifies this optimization:

Goal:

To have an Async component that accepts an async value and uses Suspense to handle that value properly.

Typical usage

<Async :value="balance"/>
setup() {
  const timer = useTimer()
  const balance = computed(() => {
     if (timer) {
      // it's alwasy true, just referring to it here to fetchBalance every time the timer ticks. 
      // similar behavior may be achieved by combining watcher and a ref
      return fetchBalance()
    }
    throw Error('Impossible')
  })
  return {
    balance
  }
}

Implementation

const AsyncChild = {
  name: 'AsyncChild',
  props: {
    value: {
      type: Promise,
      required: true,
    }
  },
  async setup(props) {
    console.log('child setup')
    const result = ref()
    watch(async () => {
      result.value = await (props.value)
    })
    await props.value;
    return () => {
      console.log('child render')
      return result.value
    }
  }
}

const Async = {
  name: 'Async',
  props: {
    value: {
      type: Promise,
      required: true,
    },
    pending: {
      type: String,
      default: "...",
    },
  },
  setup(props) {
    console.log("async setup");
    return () => {
      console.log("async render");
      return h(Suspense, null, {
        default: () => h(AsyncChild, { value: props.value }),
        fallback: () => props.pending
      });
    };
  },
};

Outcome & analysis

Using above implementation, there are 3 renders every time the timer updates:

async render
child render
// fetchBalance is running, after it's resolution
child render

Reasoning:

  1. updating value prop on Async component causes a re-render of Async component
  2. as value prop is also pass down to AsyncChild component as a prop, it also causes a re-render of AsyncChild. It also triggers the watcher in AsyncChild
  3. when the new value promise resolves, result.value is set to the newly resolved value, so AsyncChild is rendered again
  • As we're passing value prop down to AsyncChild in the render function of Async (i.e., value prop is used inside render function of Async), the first re-render is necessary.
  • As we do want to update the AsyncChild after the new promise resolves, the third re-render is necessary.
  • But the second re-render is uncessary as props.value is not directly used inside the render function of AsyncChild (only result is used)

Implementation 2

We can use a scoped AsyncChild to prevent the second re-rendering

const Async = {
  name: 'Async',
  props: {
    value: {
      type: Promise,
      required: true,
    },
    pending: {
      type: String,
      default: "...",
    },
  },
  setup(props) {
    console.log("async setup");
    // Disadvantage of this implementation:
    // We will have as many child component definition as the amount of Async component instances
    const child = {
      async setup() {
        const result = ref()
        watch(async () => {
          result.value = await props.value // note that this props is props of the Async component, not child component
        })
        await props.value
        return () => {
          console.log('child render')
          return result.value
        }
      }
    }
    return () => {
      console.log("async render");
      return h(Suspense, null, {
        default: () => h(child),
        fallback: () => props.pending
      });
    };
  },
};

Outcome & analysis

When timer updates:

async render
// fetchBalance runing, after it's resolution
child render

While, this time re-render of Async component is also redundant as props.value is no longer used in it's render function

Dream world

If an un-referenced prop update never triggers re-render, then timer update will only trigger child components re-render upon resolution of the new fetchBalance request.

Live demo

https://codesandbox.io/s/unncessary-render-on-unused-props-change-vue3-t4qov

@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants