-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Conversation
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. |
packages/reactivity/src/computed.ts
Outdated
this._setter(newValue) | ||
} | ||
|
||
get __v_isRef() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
packages/reactivity/src/computed.ts
Outdated
@@ -20,6 +20,50 @@ export interface WritableComputedOptions<T> { | |||
set: ComputedSetter<T> | |||
} | |||
|
|||
class _ComputedRef<T> { |
There was a problem hiding this comment.
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
packages/reactivity/src/ref.ts
Outdated
} | ||
} | ||
|
||
get __v_isRef() { |
There was a problem hiding this comment.
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
Great job! Wondering if vnode creation would benefit from this - let's find out? :) |
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. |
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 |
Why?
@jods4 and I noticed that
ref
s 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.
ref
&shallowRef
, benchcustomRef
, benchtoRef
, benchcomputed
, benchBesides 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
fromvue-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 totrigger
/track
. UsingtoRaw
to remedy this fixed it. This cost a tiny bit of performance, will update the benchmarks accordingly.