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

perf(ref): improve ref performance #1900

Merged
merged 5 commits into from
Aug 21, 2020
Merged

perf(ref): improve ref performance #1900

merged 5 commits into from
Aug 21, 2020

Conversation

RobbinBaauw
Copy link
Contributor

@RobbinBaauw RobbinBaauw commented Aug 19, 2020

Why?

@jods4 and I noticed that refs can be made much faster and more memory efficient by using classes (prototypes) instead of the current objects (vuejs/rfcs#197 (comment) and onwards).

What?

This changes the implementation of the following functions to a class internally. For each of the functions a benchmark is provided to show the increase in speed (typically between 70-100%). These benchmarks test pure get & set performance. There are no dependencies that need to be updated.

Besides these, I also changed the implementation of isRef to a lazy implementation, improving performance by about 10-20%. Benchmark can be found here.

Another argument to be made is the memory efficiency. I made a small gist where I create 1 million refs. When comparing the amount of memory used, I get 70MB for the class implementation against 470MB for the current implementation.

Real world

These were only tiny tests with little other computations, so one may wonder how this performs in a real world scenario.

I made small benchmark for this as well, using the useValueSync from vue-composable. This benchmark also shows a 20-30% improvement in performance!

Future

If this step is taken, we will continue our search for other occasions in which performance can be improved by converting something from an object to a class.

Edit: unfortunately the first solution failed when proxying refs. The proxied this would be passed to trigger / track. Using toRaw to remedy this fixed it. This cost a tiny bit of performance, will update the benchmarks accordingly.

@basvanmeurs
Copy link
Contributor

The original implementation creates new get and set functions on every individual ref. This explains the higher memory consumption when creating a lot of them. The optimizer is also less able to optimize it, as they are different functions.

this._setter(newValue)
}

get __v_isRef() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to make this a plain property (which would make isRef checks faster)?

I get that the getter also ensures it's not writable but I don't think anyone's actually gonna do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik plain property can be readonly too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would require explicit Object.defineProperty call though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth benchmarking the difference.
As isRef is surely not gonna be a monomorphic call site, it's most likely gonna be slow in both cases (I'd bet on the field being slightly faster, prob. not significantly).
If the browser somehow manages to create a path optimized for Ref instances, the getter is likely to be faster for the same reason as value is. Being on the prototype with trivial implementation, it might even compile to a constant in this case.

Making it a field is eating up the memory benefit, though.

Thinking this further: would we be ok to expose the Ref class as public implementation?
If we do, we could completely drop the isRef API in exchange of instanceof Ref, which is more likely to be optimized call sites (which are likely to be monomorphic, unlike isRef that is called from everywhere). That requires some benchmarking obviously.

Furthermore, we could also drop the customRef API, in exchange of class extends Ref. The resulting custom refs would be simpler to write and more performant. (This change requires putting a trigger and track on the Ref prototype, which is totally doable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm indeed, already changed this to a field but in terms of memory, we may need to revert to a getter.

Copy link
Member

@yyx990803 yyx990803 Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Maybe we can bench for memory between a field vs getter?

Exposing Ref class directly is a major API change which we are not likely going to consider at this point, as it changes how refs are created/checked and brings along all the class-related assumptions (extensions, instanceof checks) that we do not want to commit to. And I doubt the performance gain would be significant to warrant it (it might be visible in ref-only benchmarks but unlikely to matter in actual applications)

Copy link
Contributor Author

@RobbinBaauw RobbinBaauw Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go: https://gist.github.com/RobbinBaauw/dff784f226950e888a64c39febd927d0.
For me it was about a 4MB difference in favor of the prototype. This is 29MB vs 33MB (note that this is much lower than my initial test, that was on FF, this is on Chrome).

A larger difference arrives in the performance of isRef though: https://jsbench.me/cike1jpqnw. I get a 20% difference in favor of the field. Of course this will fluctuate based on what you pass, but I pass a combination of refs and non-refs so I think it is somewhat representative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we can expose the Ref but also keep current isRef.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mem cost is acceptable, let's go with field.

@@ -20,6 +20,50 @@ export interface WritableComputedOptions<T> {
set: ComputedSetter<T>
}

class _ComputedRef<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of prefixing these internal classes with _, I prefer more explicit names like ComputedRefImpl or ComputedRefConstructor

}
}

get __v_isRef() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, plain property is IMO good enough

@RobbinBaauw RobbinBaauw requested a review from yyx990803 August 20, 2020 07:49
@yyx990803 yyx990803 merged commit 07919e0 into vuejs:master Aug 21, 2020
@yyx990803
Copy link
Member

Great job! Wondering if vnode creation would benefit from this - let's find out? :)

@RobbinBaauw
Copy link
Contributor Author

Will try! Though class fields aren't saved on the prototype and VNodes don't contain many functions (if any), so I don't think it will help too much.

@zhaohui90-lee
Copy link

Why?

@jods4 and I noticed that refs can be made much faster and more memory efficient by using classes (prototypes) instead of the current objects (vuejs/rfcs#197 (comment) and onwards).

What?

This changes the implementation of the following functions to a class internally. For each of the functions a benchmark is provided to show the increase in speed (typically between 70-100%). These benchmarks test pure get & set performance. There are no dependencies that need to be updated.

Besides these, I also changed the implementation of isRef to a lazy implementation, improving performance by about 10-20%. Benchmark can be found here.

Another argument to be made is the memory efficiency. I made a small gist where I create 1 million refs. When comparing the amount of memory used, I get 70MB for the class implementation against 470MB for the current implementation.

Real world

These were only tiny tests with little other computations, so one may wonder how this performs in a real world scenario.

I made small benchmark for this as well, using the useValueSync from vue-composable. This benchmark also shows a 20-30% improvement in performance!

Future

If this step is taken, we will continue our search for other occasions in which performance can be improved by converting something from an object to a class.

Edit: unfortunately the first solution failed when proxying refs. The proxied this would be passed to trigger / track. Using toRaw to remedy this fixed it. This cost a tiny bit of performance, will update the benchmarks accordingly.

when I use the code small gist to test, the object will stable in 270MB, but when I use class, the memory cost will in crease when I repeat. why?

os: MAC OS
browser: chrome

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

Successfully merging this pull request may close these issues.

7 participants