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

feat(reactivity): more efficient reactivity system #5912

Merged
merged 225 commits into from
Oct 27, 2023

Conversation

johnsoncodehk
Copy link
Member

@johnsoncodehk johnsoncodehk commented May 13, 2022

fix #311, fix #1811, fix #6018, fix #7160, fix #8714, fix #9149, fix #9419, fix #9464

Changes

  • If computed new value does not change, computed, effect, watch, watchEffect, render dependencies will not be triggered
  • Multiple computed only trigger effect, sync watch, sync watchEffect once
  • Array shift, unshift, splice( only trigger effect, sync watch, sync watchEffect once
  • deferredComputed is now a re-export of computed and marked as deprecated
  • Added pauseScheduling, resetScheduling APIs
  • New scheduling implementation was 2.5x ~ 4.5x faster (feat(reactivity): more efficient reactivity system #5912 (comment))
Detail

computed()

let sec_counter = 0
let min_counter = 0
let hour_counter = 0

const ms = ref(0)
const sec = computed(() => { sec_counter++; return Math.floor(ms.value / 1000) })
const min = computed(() => { min_counter++; return Math.floor(sec.value / 60) })
const hour = computed(() => { hour_counter++; return Math.floor(min.value / 60) })

for (ms.value = 0;  ms.value < 10000000; ms.value += 1) {
  hour.value
}

Before result:

console.log(`sec: ${sec.value}, sec_counter: ${sec_counter}`) // sec: 10000, sec_counter: 10000001
console.log(`min: ${min.value}, min_counter: ${min_counter}`) // min: 166, min_counter: 10000001
console.log(`hour: ${hour.value}, hour_counter: ${hour_counter}`) // hour: 2, hour_counter: 10000001

PR result:

console.log(`sec: ${sec.value}, sec_counter: ${sec_counter}`) // sec: 10000, sec_counter: 10000001
console.log(`min: ${min.value}, min_counter: ${min_counter}`) // min: 166, min_counter: 10001
console.log(`hour: ${hour.value}, hour_counter: ${hour_counter}`) // hour: 2, hour_counter: 167

effect() / watch() / watchEffect()

let times = 0

const sec = ref(0)
const min = computed(() => {
  return Math.floor(sec.value / 60)
})
const hour = computed(() => {
  return Math.floor(min.value / 60)
})

effect(() => {
  times++
  min.value
  hour.value
})

for (sec.value = 0; sec.value < 10000; sec.value++) { }

Before result:

console.log(times) // 20001

PR result:

console.log(times) // 167

render()

For the following code, the template renderer is optimized to trigger once per second instead of triggering 100 times per second.

<script setup>
import { ref, computed } from 'vue'

let render_counter = 0

const ms = ref(0)
const sec = computed(() => { return Math.floor(ms.value / 1000) })

setInterval(() => {
  ms.value += 10
}, 10)
</script>

<template>
  sec: {{ sec }} ({{ ++render_counter }} render times)<br>
</template>

test cases:

Array shift()

const arr = reactive([1, 2, 3])
effect(() => {
  for (let i = 0; i < arr.length; i++) arr[i]
  console.log(arr)
})
arr.shift()

Before output:

[ 1, 2, 3 ]
[ 2, 2, 3 ]
[ 2, 3, 3 ]
[ 2, 3, <1 empty item> ]
[ 2, 3 ]

PR output:

[ 1, 2, 3 ]
[ 2, 3 ]

pauseScheduling(), resetScheduling()

Wrap refs mutations with pauseScheduling() and resetScheduling() to trigger side effects only once.

const foo = ref(0)
const bar = ref(0)

effect(() => {
  console.log(foo.value, bar.value)
})
// -> 0 0

foo.value++
// -> 1 0
bar.value++
// -> 1 1

pauseScheduling()
foo.value++
bar.value++
resetScheduling()
// -> 2 2

@johnsoncodehk johnsoncodehk marked this pull request as ready for review May 13, 2022 21:03
@johnsoncodehk johnsoncodehk changed the title perf(reactivity): calculate embedded computed() on-demand perf(reactivity): computed recalculate on-demand May 15, 2022
@skirtles-code
Copy link
Contributor

I think this may cause problems in cases that rely on the laziness of computed evaluation to avoid errors. e.g.:

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Mar 1, 2023

@skirtles-code Thanks for reviewing it, it is not a bug, users shouldn't assume that c and d is knowledge sharing, if c behaves dependent on d dependencies, d should be included by c. it's a bug

To avoid the bad architecture with circular dependencies, users should also extract the dependencies of c and d.

@yyx990803 yyx990803 force-pushed the computed-ondemand branch from e106183 to 91c0899 Compare April 5, 2023 09:40
@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot

This comment was marked as outdated.

@yyx990803 yyx990803 force-pushed the computed-ondemand branch from 91c0899 to fc84803 Compare April 5, 2023 14:33
@skirtles-code
Copy link
Contributor

@skirtles-code Thanks for reviewing it, it is not a bug, users shouldn't assume that c and d is knowledge sharing, if c behaves dependent on d dependencies, d should be included by c.

To avoid the bad architecture with circular dependencies, users should also extract the dependencies of c and d.

I'm not sure I understand the relevance of your first example. It seems you've added a circular dependency into my example. That shouldn't be there, it makes no sense.

I don't think this can be dismissed as bad architecture. The example I provided is contrived, but I was just trying to illustrate the problem. It obviously looks ridiculous when presented in this form, but that doesn't mean it wouldn't be perfectly natural to see something like this in a real application. The check on b would be something like an emptiness check or a validity check and the call to c.value would likely be something much more complicated that includes accessing c.

The underlying problem here is that, in some cases, computed properties have now switched from being lazily evaluated to eagerly evaluated. c.value is being accessed even though it isn't used. I can't find anywhere that the lazy evaluation is explicitly documented, but I think it has been the established behaviour for long enough that people have a right to expect it. In practice, I think there'll be lots of applications that implicitly rely on it, even if their developers don't realise it.

It isn't just the potential for errors. The extra evaluation of c means that, in some cases, it is doing extra, unnecessary work to evaluate d.

When evaluating d, the test for whether b.value has changed needs to be performed first, and if it has changed then there's no need to check whether c.value has changed. That would avoid the error. There's still potential for problems if the computed uses non-reactive data, but perhaps that would be acceptable. If it isn't possible to perform these change checks in the right order and bail at the right point, then I don't think this performance improvement is worth it. In the rare cases this kind of performance problem comes up it can already be worked around in userland.

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Apr 5, 2023

The underlying problem here is that, in some cases, computed properties have now switched from being lazily evaluated to eagerly evaluated.

Not really, if you find that PR causing this problem, can you provide minimal reproduction or contribute a test case?

@skirtles-code
Copy link
Contributor

The underlying problem here is that, in some cases, computed properties have now switched from being lazily evaluated to eagerly evaluated.

Not really, if you find that PR causing this problem, can you provide minimal reproduction or contribute a test case?

That is what my example shows. That is why it causes an error on this branch but not on main. It is evaluating c eagerly, even though it isn't used.

@skirtles-code
Copy link
Contributor

Here's the same example, but I've hidden the error with a ?. and put in a counter instead. Notice how c is being evaluated twice on this PR but only once on main:

@johnsoncodehk
Copy link
Member Author

It's not a trade-off, just a bug, thanks for report that.

@johnsoncodehk
Copy link
Member Author

Fixed by 88d91db (#5912)

@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Apr 6, 2023

📝 Ran ecosystem CI: Open

suite result
naive-ui ❌ failure
nuxt ❌ failure
pinia ✅ success
router ✅ success
test-utils ✅ success
vant ❌ failure
vite-plugin-vue ✅ success
vitepress ✅ success
vue-macros ✅ success
vuetify ❌ failure
vueuse ✅ success

@johnsoncodehk
Copy link
Member Author

@ferferga You need to cache and return the old object if possible.

let computed1Value

const count = ref(0)
const computed1 = computed(() => {
  const prop1 = Math.round(count.value / 1000)
  const prop2 = Math.round(count.value / 1000) * 2
  if (computed1Value && computed1Value.prop1 === prop1 && computed1Value.prop2 === prop2) {
    return computed1Value
  }
  return computed1Value = { prop1, prop2 }
})

@Anoesj
Copy link

Anoesj commented Oct 28, 2023

@ferferga Didn't understand your question at first, but I do now and created a reproduction with both the version with and without cache side by side, so others can see the difference too 😄. Interestingly, this technique also prevents rerenders in Vue 3.3.x, so I learned something new today!

@Anoesj
Copy link

Anoesj commented Oct 28, 2023

@johnsoncodehk I played around with your technique a little more and was thinking: can't this be built into computed by default? I updated the reproduction link above with a computedCached function that does this out of the box. I used lodash isEqual for now to compare the cached value with the new value and return the cache if equal. Is it possible to implement this behavior by default or would the isEqual call have a too big performance impact? If so, we could add computedCached to Vue, so people don't have to implement this technique by themselves.

@ferferga
Copy link
Contributor

@Anoesj Still, this PR reduces a lot of rerenders in my project too. I just thought it also covered objects, but I understand that's a more complex topic.

I'm not sure how that could be solved for all edge cases. Obviously pulling lodash is not a solution for Vue, so everything should be in-house. A solution must be found that is efficient enough for everybody too.

I also wonder whether something like object-hash is more efficient in this case.

@Anoesj
Copy link

Anoesj commented Oct 28, 2023

Of course, I get that. It was just a quick PoC. But having some built-in way to prevent rerenders on non-primitive computeds would be very nice.

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Oct 28, 2023

@Anoesj #9497 can reduce a bit of boilerplate code.

We cannot built-in isEqual for object. In addition to performance issues, we should not assume that objects with the same properties should produce the same behavior in user logic.

@johnsoncodehk
Copy link
Member Author

@ferferga the object-hash's approach is great! I think we can have an object-hash based computed API in vueuse. cc @antfu

@ferferga
Copy link
Contributor

ferferga commented Oct 28, 2023

@johnsoncodehk What are the difficulties you see to have it in core? It's great that we can finally have a one-size-fits-all approach to computed and no longer need computedEager, so I think it'd be great too to not have other comp's implementation

I understand your concerns of more potential breakage, but maybe for 3.5?

PS: Another simpler approach maybe is to stringify? Not sure if that could have it's edge cases too... This is surely a complex problem.

@Anoesj
Copy link

Anoesj commented Oct 28, 2023

@johnsoncodehk Nice, that would also work. Also, one hour ago? Dude, you're beast, haha!

@Anoesj
Copy link

Anoesj commented Oct 28, 2023

@ferferga Equality based on stringify wouldn't be optimal when objects have the same properties and values, but in a different order. When working with Array, Map and Set it makes sense to not count something as "equal" when the order is different, but with objects, in my opinion, it's not. Don't know how object-hash handles order though? Maybe, if a non-primitive equality check functionality is made available, there needs to be an opt-in setting to ignore order? Also curious how two class instances with the exact same properties, but a completely different prototype, should be handled.

@beskar-developer
Copy link

@johnsoncodehk
Hi. I have a scenario that has a different outcome in different versions of vue!

<template>
  {{ items }}
</template>

<script setup>
  import { computed, reactive, watch } from "vue";

  const someArray = [Math.random(), Math.random(), Math.random()];

  const someReactive = reactive({
    dir: "asc",
    execute(items) {
      return items.sort((a, b) => (this.dir === "asc" ? a - b : b - a));
    },
  });

  const items = computed(() => {
    const sortedItems = someReactive.execute(someArray);

    console.log("computed triggered", sortedItems);

    return sortedItems;
  });

  watch(
    items,
    () => {
      console.log("watch is triggered", items.value);
    },
    {
      deep: true,
    },
  );

  setTimeout(() => {
    someReactive.dir = "desc";
  }, 1500);
</script>

In vue 3.3 the computed, watch, and render are triggered and the array is changed.
In vue 3.4 the computed is triggered but watch and render do not!.
I think this is some sort of BREAKING CHANGE behavior if it is not a bug.
The hasChanged function that compares two values is just looking for a reference that does not work properly in this scenario.

@skirtles-code
Copy link
Contributor

@beskar-developer. This PR was merged a long time ago. If you think you've found a bug then it'd be better to open a new issue at https://github.com/vuejs/core/issues. If you're unsure whether it's a bug you could also discuss it at either https://github.com/vuejs/core/discussions or on Discord first.

In my opinion, this is neither a bug nor a breaking change, but I'd prefer not to go into details on this PR.

@basvanmeurs
Copy link
Contributor

@johnsoncodehk Hi. I have a scenario that has a different outcome in different versions of vue!

<template>
  {{ items }}
</template>

<script setup>
  import { computed, reactive, watch } from "vue";

  const someArray = [Math.random(), Math.random(), Math.random()];

  const someReactive = reactive({
    dir: "asc",
    execute(items) {
      return items.sort((a, b) => (this.dir === "asc" ? a - b : b - a));
    },
  });

  const items = computed(() => {
    const sortedItems = someReactive.execute(someArray);

    console.log("computed triggered", sortedItems);

    return sortedItems;
  });

  watch(
    items,
    () => {
      console.log("watch is triggered", items.value);
    },
    {
      deep: true,
    },
  );

  setTimeout(() => {
    someReactive.dir = "desc";
  }, 1500);
</script>

In vue 3.3 the computed, watch, and render are triggered and the array is changed. In vue 3.4 the computed is triggered but watch and render do not!. I think this is some sort of BREAKING CHANGE behavior if it is not a bug. The hasChanged function that compares two values is just looking for a reference that does not work properly in this scenario.

This computed sorts someArray in place, which means it always returns the same reference. Since 3.4 computeds have become lazy. If the result is equal (as in this case, given it is the same reference) it will not trigger deps.

Solution may be to return a new array when using the sort function, using return [...sortedItems] in the computed.

@beskar-developer
Copy link

@johnsoncodehk Hi. I have a scenario that has a different outcome in different versions of vue!

<template>
  {{ items }}
</template>

<script setup>
  import { computed, reactive, watch } from "vue";

  const someArray = [Math.random(), Math.random(), Math.random()];

  const someReactive = reactive({
    dir: "asc",
    execute(items) {
      return items.sort((a, b) => (this.dir === "asc" ? a - b : b - a));
    },
  });

  const items = computed(() => {
    const sortedItems = someReactive.execute(someArray);

    console.log("computed triggered", sortedItems);

    return sortedItems;
  });

  watch(
    items,
    () => {
      console.log("watch is triggered", items.value);
    },
    {
      deep: true,
    },
  );

  setTimeout(() => {
    someReactive.dir = "desc";
  }, 1500);
</script>

In vue 3.3 the computed, watch, and render are triggered and the array is changed. In vue 3.4 the computed is triggered but watch and render do not!. I think this is some sort of BREAKING CHANGE behavior if it is not a bug. The hasChanged function that compares two values is just looking for a reference that does not work properly in this scenario.

This computed sorts someArray in place, which means it always returns the same reference. Since 3.4 computeds have become lazy. If the result is equal (as in this case, given it is the same reference) it will not trigger deps.

Solution may be to return a new array when using the sort function, using return [...sortedItems] in the computed.

Thanks but what I meant was this could be a breaking change, some bug, or maybe nothing!
I have opened a discussion about this matter #11193.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment