Beta dependency tracking executes watchers with inconsistent values

[Frizi]

Version

2.5.17-beta.0

Reproduction link

https://jsfiddle.net/w5d9gqmo/3/

Steps to reproduce

In provided example:

• open console
• hit increment
• observe computed value evaluations in console

In vue terms:

• create dependency structure with many paths to the same watcher (A->B, B->C, A->C)
• Notify leaf watcher about change (data change)
• Observe too eager synchronous updates of watchers on every intermediate change

What is expected?

All updated computed values are executed once and with correct dependant values.

twoCounts evaluated to: {count: 2, countPlusOne: 3}

What is actually happening?

Computed values are executed multiple times with all intermediate dependency values.

twoCounts evaluated to: {count: 2, countPlusOne: 2}
twoCounts evaluated to: {count: 2, countPlusOne: 3}

---

The issue is clearly caused by this commit: 653aac2

I did some debugging and found the cause.
Dependencies are notified synchronously about the watcher update in the notify loop, but the updating watchers might be accessed too early with stale value and no information about necessary recomputation.

I attempted a fix by splitting Dep.notify into two phases with separate loops for dirtify and update. It worked for simple situations where there is triangle of dependencies (countPlusOne in the example), but it stops working when the graph is any more complicated (paths are larger than 1). This is illustrated by countPlusTwo computed in the example.

Possible full fix would involve either traversing the full dependency graph to dirtify all deep dependants first, or collecting the the array of dependants of my dependants like in previous implementation.

[haoqunjiang]

As a sidenote:
This commit also causes bugs in the DatePicker component of element-ui.
https://circleci.com/gh/sodatea/regression-test-prototype/79

[yyx990803]

This has been reverted in 6b1d431.

The original intention of the change was to avoid unnecessary re-renders when a computed property's dependencies changed, but the computed value remains unchanged. However, it requires computed properties with dependencies to become "eager", and re-evaluate synchronously whenever one of its dependencies change. This leads to the issue described here.

Chained computed properties are pretty common, and most likely more common than the case we originally tried to prevent (computed property deps changed but value remains same). Most importantly, the original behavior does not lead to duplicated computation even in the worst case scenario, while the now-reverted behavior could lead to potentially much more wasted CPU cycles. So we are reverting this change.

[LinusBorg]

Most importantly, the original behavior does not lead to duplicated computation even in the worst case scenario, while the now-reverted behavior could lead to potentially much more wasted CPU cycles. So we are reverting this change.

While that's probably true statistically, it's still unfortunate for situations where a computed property does some heavy calulation over and over because a previous computed property often updates, but always returns the same value.

I guess in these situations the user must do some manual caching in this computed property, or use a memoized method instead?

[indirectlylit]

it's still unfortunate for situations where a computed property does some heavy calulation over and over because a previous computed property often updates, but always returns the same value

Yes, this was exactly the situation I reported in #8540 (before seeing that it was a duplicate). I was trying to use computed props as an elegant way to cache intermediate results.

The actual behavior was surprising to me, and there may be others out there who are constructing their computed props as "intermediate caches" with expectation of better performance than they're getting due to incorrect belief about re-compute behaviors.

I guess in these situations the user must do some manual caching in this computed property, or use a memoized method instead?

Yes, this is what I'll need to do instead. Would love if eventually this memoization behavior was built in to computed props!

[indirectlylit]

FYI our use-case was to prevent certain calculations from happening on rapid events. For example:

{
  data() {
    return {
      width: 0, // updated on every resize event
    }
  },
  computed: {
    isMobile() { return this.width < 400; },
    dynamicStyle() {
      if (this.isMobile) {
        // expensive stuff
      }
      // expensive stuff
    },
  }
}

[Frizi]

@indirectlylit You can accomplish this by caching the state by yourself.

  data() {
    return {
      width: 0, // updated on every resize event
      isMobile: undefined, // let it be handled by "immediate" watch
    }
  },
  watch: {
    width: {
      immediate: true,
      handler () {
        this.isMobile = this.width < 400
      }
    }
  },
  computed: {
    dynamicStyle() {
      if (this.isMobile) {
        // expensive stuff
      }
      // expensive stuff
    },
  }
}

Note that this illustrates the most generic pattern, but in your case you can probably just update isMobile inside the resize event anyway (and you probably should, less code and much simpler solution).

This is almost equivalent to what the previous buggy dep tracking was doing - making an eager recompute. This is that way because every watch or render (template code) is essentially forcing it's recomputation on any change, in other words it's always eager. On the other hand, the computed value is always waiting until explicitly requested, in other words it's always lazy. I personally find that much simpler to reason about.

If I could propose any potential solution to that, I think it might be viable to add something like lazy: false as an additional computed option, which would effectively translate to the pattern above.

Like so:

// this is a proposal, not a working code
computed: {
 isMobile: {
   lazy: false,
   get () { return this.width < 400; }
 }

[indirectlylit]

Thanks @Frizi!

Yup, adding data and watchers was exactly what I ended up doing.

The example above was a simplification of our actual use-case; you can see the full code change here for context.

I appreciate that this might be a case where implementation details (lazy vs eager evaluation) might need to bubble up to the Vue API and be exposed in some way.

In an ideal world though what I'd love is kind of a combination that gets used under-the-hood: eagerly set 'dirty' flags without recomputing, and lazily recompute dirty values when requested.

(I don't know enough about the dep tracking internals in Vue to know how feasible this is in practice...)

[indirectlylit]

In an ideal world though what I'd love is kind of a combination that gets used under-the-hood: eagerly set 'dirty' flags without recomputing, and lazily recompute dirty values when requested.

re-reading your original issue, I see this is very similar to what you already said :)

Possible full fix would involve either traversing the full dependency graph to dirtify all deep dependants first, or collecting the the array of dependants of my dependants like in previous implementation.

[Frizi]

@indirectlylit

In an ideal world though what I'd love is kind of a combination that gets used under-the-hood: eagerly set 'dirty' flags without recomputing, and lazily recompute dirty values when requested.

This is exactly what's happening in standard computed values 😄

Computed values are lazy watchers, which mean that on dependency change, a dirty flag is set.

vue/src/core/observer/watcher.js

Lines 159 to 172 in 0737d11

	/**
	* Subscriber interface.
	* Will be called when a dependency changes.
	*/
	update () {
	/* istanbul ignore else */
	if (this.lazy) {
	this.dirty = true
	} else if (this.sync) {
	this.run()
	} else {
	queueWatcher(this)
	}
	}

Once a value is requested, the cached one is used or it is recomputed when dirty.

vue/src/core/instance/state.js

Lines 244 to 255 in 0737d11

	return function computedGetter () {
	const watcher = this._computedWatchers && this._computedWatchers[key]
	if (watcher) {
	if (watcher.dirty) {
	watcher.evaluate()
	}
	if (Dep.target) {
	watcher.depend()
	}
	return watcher.value
	}
	}

The important part is that transitive dependencies are always treated like direct dependencies. In means if a computed value A calls computed value B, all dependencies of B are copied to A. This is done like so, because when B's dependency change and you request value of A, it has to be recomputed or it risks being outdated.

vue/src/core/observer/watcher.js

Lines 214 to 222 in 0737d11

	/**
	* Depend on all deps collected by this watcher.
	*/
	depend () {
	let i = this.deps.length
	while (i--) {
	this.deps[i].depend()
	}
	}

Changing that behaviour to first check if B was actually changed is not that simple, as you have to keep that deferred up to the point when A is requested (or any other dependant computed value), while caring about A to not recompute if possible.
I think that this is achievable, but the complexity is much bigger and in many cases it might turn out that benefits outweighs the costs, both in complexity and speed. In today's hardware, the memory fetch is usually the slowest action you can take, so caching every trivial computation like that can actually be a serious performance hog. There is still a place for methods :D