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

Autotracking Memoization #615

Merged
merged 10 commits into from
May 8, 2020
Merged

Autotracking Memoization #615

merged 10 commits into from
May 8, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 18, 2020

@BryanCrotaz
Copy link

Looks good. Solves my usages of observes, for example playing a video element on a data change.

@BryanCrotaz
Copy link

This should be a decorator?

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 23, 2020

@BryanCrotaz no, this is meant to be a low-level API only. Decorators like @cached will be based on it (I need to update the text of that RFC, have updated the name already), and it would also allow us to build things like the @use and Resources RFC in userland for the most part.

@richard-viney
Copy link
Contributor

Couple minor typos in the first line of the usage section: cache => createCache(), and "an cache" => "a cache".

Chris Garrett and others added 2 commits April 28, 2020 10:08
Co-Authored-By: Scott Ames-Messinger <scott@commoncurriculum.com>
@rwjblue
Copy link
Member

rwjblue commented May 1, 2020

We discussed this in todays core team meeting, and are moving it into final comment period. Time to read through it again! 😸 👓 🤓

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

I really like this RFC and well written. Some of the low level primitives that have been exposed recently have been immensely useful. So along the same track, this seems like a great idea!

text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Show resolved Hide resolved
text/0615-autotracking-memoization.md Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
Chris Garrett and others added 2 commits May 8, 2020 08:50
Co-authored-by: Robert Jackson <me@rwjblue.com>
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Let's remove the isCache API for now (if we have a use case that comes up later, we can definitely add it).

text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
text/0615-autotracking-memoization.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Jackson <me@rwjblue.com>
@rwjblue rwjblue merged commit f0a9b20 into master May 8, 2020
@rwjblue rwjblue deleted the autotracking-memoization branch May 8, 2020 19:32
@wycats
Copy link
Member

wycats commented May 11, 2020

For historical reference:

I was momentarily worried that this RFC doesn't provide an easy way to check whether a computation is const without creating unnecessary garbage.

This problem isn't semantic: it's not a semantic problem to create a Cache and immediately throw it away. I was worried, at first, that this API would make it difficult to implement Glimmer's strategy of running a computation, checking if it was const, and only bothering to cache the value at all if the value wasn't const.

@pzuraq persuaded me that the current spec allows this implementation: the implementation could check to see if the computation is const and not bother to store the value at all if it is.

This whole thing only matters if the cost of storing the cache momentarily is a notable cost. If it turns out to be a serious issue, and @pzuraq's thinking turns out to be wrong, we could always revisit this issue.

@rwjblue rwjblue mentioned this pull request May 18, 2020
@rwjblue
Copy link
Member

rwjblue commented May 18, 2020

FYI - @pzuraq created a polyfill for this that works back to ember-source@3.13 (going further back is very very difficult since autotracking in general is missing).

Check it out https://github.com/ember-polyfills/ember-cache-primitive-polyfill

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

Successfully merging this pull request may close these issues.

7 participants