-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
@cached #566
@cached #566
Conversation
not change. Unfortunately, this is not really something that `@memo` can | ||
circumvent, since there's no way to tell if a value has actually changed, or | ||
to know which values are being accessed when the memoized value is accessed | ||
until the getter is run. |
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.
This is a bit confusing. If the purpose of @memo
is to not re-reun the getter unless the underlying properties have changed, how can the statement "may rerun even if the values themselves have not changed" be true? It seems that it's implying that any set operation invalidates the @memoized
value, but I don't see that stated elsewhere in this RFC. Maybe it would be good to add that if it's true?
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.
Yes, and that was specified in the original tracked properties RFC: https://github.com/emberjs/rfcs/blob/master/text/0410-tracked-properties.md#manual-invalidation
This clause basically is stating that @memo
cannot possibly know whether a value has actually changed. All it can know is if the tags have changed, and it will defer to the change semantics that were defined upstream. In the future, we could change @tracked
to check if the value is different potentially, and not invalidate unless it was. We could also add a new decorator which does this.
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.
In the future, we could change
@tracked
to check if the value is different potentially, and not invalidate unless it was. We could also add a new decorator which does this.
This would be very interesting to see!
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.
Yes, and that was specified in the original tracked properties RFC:
Hmm, so the RFC for "tracked" may state how invalidation works, but to me, the whole purpose of a @memo
decorator is to change how invalidation works. That's why I felt it would be appropriate to say more here about what causes a value to be returned from the cache and what causes the getter to re-run.
Maybe a code example would help:
class Foo {
@tracked bar;
@memo
get modBar() {
return this.bar + 1;
}
}
const foo = new Foo();
foo.bar = 1;
console.log(foo.modBar);
foo.bar = 1;
console.log(foo.modBar);
With this code, the modBar
getter will re-evaluate both times it is accessed even though foo
has not changed value. (laying it out explicitly since I'm still only 95% sure that that's what the RFC is trying to say)
Looks great! Couple things:
|
This is a bit of an orthogonal concern, and not really in the scope of this RFC I think. We are definitely working on better debug tooling here though, I think a future RFC is definitely in order!
This is not possible with the autotracking system, since invalidation is only observable later on when the value is consumed. So, it would basically be sending the event the first time (if ever) that the memoized value was read. Fundamentally, this is just not something that lazy, pulled based change tracking can accomplish. |
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.
This is a great RFC and will be super beneficial to the community!
} | ||
``` | ||
|
||
In this example, the `fullName` getter will be memoized whenever it is called, |
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.
the fullName getter return value will be memoized
Although perhaps both say the same thing.
Cycles will not be allowed with `@memo`. The cache will only be activated | ||
_after_ the getter has fully calculated, so any cycles will cause infinite | ||
recursion (and eventually, stack overflow), just like un-memoized getters. If a | ||
cycle is detected in `DEBUG` mode, it will throw an error. |
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.
This is good information!
A few other questions I had. It seems this is a per instance memoization technique? Will this only cache the last result (for memory safety)? Do you think these are relevant points or just implementation details that don't need to be flushed out at this time?
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.
This definitely would be per-instance, and it would capture only the last result. I think we can specify it here 👍
- Adds extra complexity when programming (whether or not a value should be | ||
memoized is now a decision that has to be made). In general, we should make | ||
sure this is not an issue by recommending that memoization idiomatically _not_ | ||
be used _unless_ it is absolutely necessary. |
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.
A few spots you discussed potential downside but I'm not sure I grasped them simply from reading. Is it cache space, performance, complexity? (and sorry if I didn't read carefully enough)
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.
Every @memo
d property means.
- Slightly more startup cost, to decorate the property
- Slightly more memory cost, to cache the values
- Slightly more runtime cost to calculate the value, since it is wrapped in another function now
- Slightly more cost to revalidate on every render, even if nothing has changed.
These costs are tiny, but if every getter in an app was decorated, they would add up.
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.
Can you add this information to the How we teach this
section, so it doesn't get lost accidentally? I think this is vital to understanding when to @memo
or not to @memo
.
Maybe the advanced guides could also go into detail about how to performance profile this. Can we make this info easily accessible in the Ember Inspector maybe?
and will only be recalculated the next time the `firstName` or `lastName` | ||
properties are set. This would apply to any autotracking tags consumed while | ||
calculating the getter, so changes to `EmberArray`s and other tracked | ||
primitives, for instance, would also cause invalidations. |
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.
@tracked arr = [1, 2, 3];
Perhaps it was only not clear to me but would changing arr
to [1,3,4]
also invalidate the cache (using your tracked-built-ins
)? Or only if a new arr
is set?
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.
That’s exactly what that was meant to capture. Essentially, any autotrack-able change should bust the cache. If it would invalidate the template, it would invalidate @memo
.
I like ‘@cached’ better. |
assigning the property: | ||
|
||
```js | ||
if (newValue !== this.trackedProp) { |
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 especially like this requirement, because equality gets fuzzy when you start working with non-primitives.
In the documentation for this, I think it'd be good to include examples of complex equality checking through layer of tracked objects to demonstrate that it would not be possible to performantly check value diffs in any memorization implementation?
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 also like that this is laid out in the RFC, but I think it should be part of the the How We Teach This section also. In my previous experience (with Ruby), a memoized value does not change unless the cache itself is reset. Whereas in this case, it's unexpectedly(?) resetting when an upstream value is set. I wonder of either I understand memoization incorrectly or that it really can mean different things? Here's an example of how I've memoized in Ruby:
class Foo
def bar
@_bar || some_expensive_method
end
private
def some_expensive_method
# some expensive code
end
end
the memoized return value of Foo#bar
doesn't depend on anything else other than the cached instance variable @_bar
.
I like this a lot. |
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.
Awesome!
text/0566-memo-decorator.md
Outdated
`@memo` is not an essential part of the reactivity model, so it shouldn't be | ||
covered during the main component/reactivity guide flow. Instead, it should be | ||
covered in the intermediate/in-depth guides, and in any performance related | ||
guides. |
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.
Maybe a cross-link in parentheses should still be added? A question that I would immediately ask myself when reading the explanation for regular getters and @tracked
: "Isn't this super inefficient?"
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.
Really? I'm going to be doing some talks about it soon, but yeah the strategy is actually quite efficient, at least compared to similar frameworks like MobX and Vue's data
stuff, where they all use observables/streams under the hood.
I think we should probably add a section once we get public autotracking primitives (which hopefully will be sooner rather than later)
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.
Yeah, I guess it's because I'd be coming in with a pre-Octane mindset, where everything was memoized by default and it just "felt right", because you seem to be saving unnecessary work. The fact that the actual memoization itself also takes time is hidden away and not immediately apparent to the user.
I'm super thrilled to see these talks!
- Adds extra complexity when programming (whether or not a value should be | ||
memoized is now a decision that has to be made). In general, we should make | ||
sure this is not an issue by recommending that memoization idiomatically _not_ | ||
be used _unless_ it is absolutely necessary. |
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.
Can you add this information to the How we teach this
section, so it doesn't get lost accidentally? I think this is vital to understanding when to @memo
or not to @memo
.
Maybe the advanced guides could also go into detail about how to performance profile this. Can we make this info easily accessible in the Ember Inspector maybe?
not change. Unfortunately, this is not really something that `@memo` can | ||
circumvent, since there's no way to tell if a value has actually changed, or | ||
to know which values are being accessed when the memoized value is accessed | ||
until the getter is run. |
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.
In the future, we could change
@tracked
to check if the value is different potentially, and not invalidate unless it was. We could also add a new decorator which does this.
This would be very interesting to see!
text/0566-memo-decorator.md
Outdated
|
||
## Detailed design | ||
|
||
The `@memo` decorator will be exported from `@glimmer/tracking`, alongside |
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 understand that
- this is a very
@glimmer/tracking
-specific feature - that is deeply connected to the tracking system
- and embedding it there
- standardizes the implementation across all apps
- and also significantly reduces the time to market
But just so the question was asked: Was is considered to open up the tracking / tags API instead to allow user-land modules to implement @memo
and other decorators?
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.
Yes! I actually have another RFC in the works to do just this 😄 I wanted to land @memo
because 1. it's a very small RFC, took half a day to write and I don't think it's super contentious and 2. I do think we should have a conventional solution for this in the framework, enough folks have asked for it that it would be a pain to have to install from the community.
But I 100% agree, we should have public tracking primitives that you could use to build something like @memo
on top of!
Please state explicitly:
where emberDataModel is something that contains cycles and therefore cannot be and
|
Most memoization functions you find online serialize their arguments to create a key for a cache. |
Ah, yes this won’t be an issue with this decorator. It’s not memoizing based on arguments, it’s memoizing based on state consumed during calculation. This could potentially be expanded in the future, and we would address the issue then. |
I think this is great but I feel the name is confusing. |
For developer ergonomics |
Is there an opportunity here for some instrumentation so the developer can determine if the memoization was "worth it"? Knowing the maximum (and perhaps minimum) number of times a memoized value is read between writes is useful data and is relatively cheap to calculate. |
In this case, we could consider "memoized" as well, cause it's aligned with "computed" and "tracked" stylistically. And I think |
We discussed this on the core team a while back, sorry I haven't updated. This was a major consideration, keeping the theme consistent for all of the decorators was something we wanted to make sure we did. The consensus was that |
Do we need:
|
@lolmaus I don't think so, and I think it could lead to a lot of antipatterns actually. I've already been seeing developers try to fallback to eventing and push-based reactivity at times when it wasn't really necessary, but they weren't familiar enough with the mental model of autotracking yet to see a better solution. With computed properties, manual invalidation was sometimes necessary because the system could not express certain chains of dependencies. A great example of this (which I intend to publish a deep dive blog post about) is the This is something that is trivial with autotracking - you don't even need to think about it, it already just works. We can also always revisit this is the future if we find that there are certain patterns which require more manual/direct control. I think for those cases though, autotracking primitives (currently working on the RFC) will suffice, and don't need to integrate with |
@pzuraq Thank you for the explanations. 🙇♂️ |
I'm very concerned about this part of the RFC:
This reminded me of my experience with AngularJS 1. I had a restricted page, a user should've been able to visit it only when they matched certain conditions. But the framework did not allow me to make this check in a central place (like Ember If I forgot to add a check to a link, users would be able to visit the restricted page when they shouldn't have been allowed to. This was also introducing multiple sources of truth: when refactoring the restrictions, it was easy to introduce inconsistencies. Similarly, I would like to make a decision whether the getter should recompute or not when dependent value is the same — on the getter level and not on the level of each setter of each It's really not the responsibility of
For these reasons, I believe it should be possible to decide whether a getter should recompute on unchanged dependencies — on the level of the getter and not its dependencies. I can imagine something like this: @cached({shouldUpdateOnUnchangedDependencies: false})
get foo() {
return cpuIntensiveComputation(this.foo, this.thirdPartyService.bar.baz.quux);
} Please note that I do not insist that this functionality must be necessarily part of Ember core. I don't care if I import it from What I do find important though is that the RFC should account for this use case and, if it's not supposed to be part of Ember core, provide a reference implementation for addons to pick up, rather than saying "you can use any third-party library, go figure it out yourself". That just doesn't work. The reason why I started thinking about it is that I've got a few project where Redux store is used, but without I have a Redux store and various computed properties depend on values deeply nested in the store. The problem is that any change to the store would cause all getters that depend on it to recompute — even though those getters depend on small, unchanged properties deeply nested within the store tree. I'm looking for a way to prevent that. With the old paradigm, I had to explicitly provide dependant keys to How do I do that with the new paradigm of |
@lolmaus I agree. It should be the responsibility of the getter that does the heavy computation to diff its direct dependents. The relevant section should be changed. Instead, rewrite the getter like this: @cached
get foo() {
let quux = this.thirdPartyService.bar.baz.quux;
if (this.bar === this.oldBar && quux === this.oldQuux) {
return this.cachedFoo;
}
this.oldBar = this.bar;
this.oldQuux = quux;
this.cachedFoo = cpuIntensiveComputation(this.bar, quux);
return this.cachedFoo;
} Of course, after going through all this trouble, you no longer need |
@Gaurav0, I've also been thinking about going this path, except that I don't like side-effects and the amount of boilerplate. Instead, I was thinking about an API similar to But as this RFC correctly admits, the ergonomics of providing keys are far from perfect. It would feel kinda a step back from the path Octane is taking. I believe, that this RFC should attempt resolving it with tracking magic. There should be a way to simply opt into not recomputing if dependent values haven't changed. E. g. by passing Ideally, in addition to not performing the calculation, it should also interrupt the propagation chain. And yes, as @Gaurav0 mentions, if this PR leaves this problem out of its scope and suggest using a third-party decorator for opting out of recomputation, such third-party decorator would defeat the purpose of this RFC. And it will do so with a small overhead, maybe at the cost of ergonomics. |
@lolmaus what you describe is not possible in autotracking, unfortunately. The system is based around weak signals, not strong ones. By the time we get to a given Like @Gaurav0 pointed out, the best I think we can do here is some amount of manual cache validation. This could be built into a future version of the decorator, but we should experiment with the API first. I think getting the weak-cached version out sooner rather than later is probably good in the meantime. |
This should not be a problem if you actually track root state in general. The root state is not the store - it's the small, unchanged properties. If you use a library like tracked-built-ins rather than signaling that the entire store has changed every time, then this will be much less of a concern in general. This is part of the reason why I'm pushing for tracked built-ins to be part of the system. Root state should be always be annotated properly, and you shouldn't be signaling changes via parent objects. This is the flaw that Redux (and indeed React) have when it comes to state management - no way to narrow the scope of a change easily. We do have that, it just means you need to annotate the proper state. |
@pzuraq Thank you for acknowledging the legitimacy of my concerns, explaining why it can't be solved with autotracking magic and suggesting an alternative solution. Using This indeed seems reasonable and aligns well with Ember's traditional OOP approach. And this is exactly what @lifeart told me to do when I was whining about this RFC not helping me with my Redux woes. Unfortunately, in my personal case, Redux is an integral part of all projects I'm currently involved with. The reasons Redux was chosen is because:
And I'm not the only one. Redux is too big of player to be turned a blind eye upon. The fact of Redux being troublesome/fiddly to use in Ember — will surely not help with increasing Ember's popularity on the frontend frontier. PS I'm not saying you're wrong in any regard. And I'm certainly not saying this RFC is acting against Ember interest! But I do feel worried that the new autotracking paradigm would not cover all the cases of the former, soon-to-be-deprecated paradigm. PPS My comment kinda implies that Redux is well-supported by the classic Ember paradigm. It isn't! But boy, would it be awesome to simply inject a Redux service and have your getters directly depend on nested store properties, without funky business like |
I'm sorry if I made it seem this way, but that is definitely not what I meant! What I moreso mean is that, ultimately, every piece of state in a JS app ultimately boils down to:
So no matter what your implementation is, you should be able to effectively wrap it somehow, and once it's instrumented only track the deltas. This is the goal of autotracking in general. This is why For the case of Redux, we would want to do something similar. Basically, like you said here:
That should be completely possible! It would just mean wrapping Redux such that the edge/leaf properties entangle properly, and then notifying when they change. How we choose to notify and entangle depends on the library - for Redux, it may end up being more bespoke and require manual wrapping. Maybe it could use This is what I mean when I keep saying that autotracking is paradigmless. It is a tiny wrapper on top of the state primitives in JS - properties and collections. Literally every type of data library is built on those, so every library should be wrappable in this way. |
@pzuraq That's awesome! Can we come up with a specific solution? I can imagine a decorator that follows ergonomics of get expensive() {
return cpuIntensive(this.store.foo.bar.baz.quux);
}
@myReadOnly('store.foo.bar.baz.quux')
quux;
get cheap() {
return cpuIntensive(this.quux);
} Alternatively, a decorator could be applied to a getter, with arguments similar to
The latter feels kinda like alternative to this RFC, with the Classic API. @pzuraq, how can Not sure how PS OK, now that you have clearly explained the boundaries of what autotracking can and cannot do, this matter indeed feels as an offtopic in this thread. But I feel really uncomfortable hopping onto the autotracking train, leaving Classic paradigm behind for good, without matters like this being fully resolved. |
I’d need to spend some time digging into how Redux is implemented, but I would naturally want to put the autotracking on the store-side, rather than the consumer side. Ideally you would just inject the store and reference it like a normal property: @cached
get cheap() {
return cpuIntensive(this.store.foo.bar.baz.quux);
} How we do that really again depends on how the store is implemented. It gets a bit trickier if it really is immutable, since we’ll have to do diffing ourselves then or somehow keep track of what exactly has changed. I would likely do it via a Proxy though, for starters, since that keeps us pretty flexible. |
Now that #615 is landed, I think this is ready for a revamp... |
This adds a mapping from `{ memo } from '@glimmer/tracking'` to `Ember._memo`. This enables the [`ember-memo-decorator-polyfill`][polyfill] for [RFC 566 "@memo decorator"][rfc-566] to work without any [`patch-package`][patch-package] hackery. [polyfill]: https://github.com/ember-polyfills/ember-memo-decorator-polyfill [rfc-566]: emberjs/rfcs#566 [patch-package]: https://github.com/ds300/patch-package/issues
You can already start experimenting with this using the |
- RFC PR: https://github.com/emberjs/rfcs/pull/566 | ||
- Tracking: (leave this empty) | ||
|
||
# @cached |
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.
# @cached | |
# Add a `@cached` decorator |
I think the title should be more descriptive.
We discussed this in todays core team meeting, and are generally in favor of moving forward here. There are a couple of things that we should update with before FCP'ing:
@pzuraq - Would you mind updating with ^ info? |
8900bb1
to
7404b44
Compare
Thanks for updating @pzuraq! We discussed this at todays core team meeting, and are still very much in favor of moving forward. Moving this into final comment period... |
``` | ||
|
||
In this example, the `fullName` getter will be memoized whenever it is called, | ||
and will only be recalculated the next time the `firstName` or `lastName` |
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.
@pzuraq is the opposite true-- if your cached getter relies on zero tracked properties, will the getter just calculate once and then not recalculate?
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.
@roneesh, a @cached
getter that does not read any @tracked
properties (directly or through other getters) — will never be recalculated.
But I believe this behavior is not opposite to the one you quoted.
Rendered