-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add snapshottedViews #103
Add snapshottedViews #103
Conversation
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.
shimp it
I want to get the Gadget side PR ✅ before merging this in-case I need to make more changes. |
CodSpeed Performance ReportMerging #103 will improve performances by 19.79%Comparing Summary
Benchmarks breakdown
|
40b2cc9
to
5e61c01
Compare
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.
🚀
[no-changelog-required]
5e61c01
to
48c5542
Compare
This is a subset of #69 that only includes re-hydrating
@snapshottedView
's from a snapshot. This does not include storing the result of a@snapshottedView
's value into a snaphot viagetSnapshot
-- that must be done manually.Gadget side PR showing how it might be used: https://github.com/gadget-inc/gadget/pull/11994
Below is a revised version of #69's description
This adds a feature for caching the output of a view in the snapshot of a node. For expensive views, it's nice to compute them at write time, serialize their return values, and then deserialize them in all the readonly contexts instead of computing them. For things like Gadget's action options (or even the string operations to produce the api identifier), we're already computing them once, we can cache that through the snapshot and avoid recomputing them on every read only root. Not all views are snapshotted; you opt in with a
@snapshottedView()
decorator.As an example, say we have this class:
We can include the
@snapshottedView
's value in the snapshot:Then, if I create a readonly instance from that snapshot, I can ask for the slug, and the cached value will be returned right away, without calling the view function:
Yay!
The details
The devil for this one is in the deep and frankly super intense details about how and when exactly these computations happen. Here's some decision points:
Must all snapshots have the value for all snapshotted views?
I vote no so that this feature is incrementally adoptable. If you have a bunch of stored snapshots without a view in them, and then switch a view over to be snapshotted, ideally, the old stored snapshots still work. We have a way to compute the view in both read-only and observable instances that already has nice pure / safe properties, so I think these properties should be optional in the snapshot.
What if the definition of the view changes such that recomputing it would produce a different value than the snapshot?
A great question. Rephrased, this is also asking "what if the incoming snapshot lies about the view value", and the answer is that detecting lies is expensive. We could recompute the view to see if it is correct, but that defeats the performance-win purpose of the cache in the first place. We could store some version number or sigil in the snapshot itself, and only use the value if the version matches, but that seems complicated, and like it would cause some surprise performance issues where code changes all of a sudden caused the caches to stop hitting, but not necessarily cause them to be re-filled with up to date data, getting us stuck in a poor-er performing spot.
So, I opted to just trust the snapshot, and rely on whatever the thing feeding in the snapshot is to feed in a new one when required. In Gadget's case, the environment version mechanism around a ROSR cache should serve this purpose well, but not everyone else is gonna have that. I think this is a super advanced feature that is ok to have barbs like this.
Should observable instances care about the value in the snapshot?
For read-only instances, we should use the snapshot value to avoid running the view and improve performance. But for observable instances, we could "seed" the view with the initial value from the snapshot, or we could just ignore it, and recompute on demand. I think that because of the above view-definition/snapshot-value drift problem, we should do this computation and take the hit. It's also kinda complicated to seed the value of a computed like this, but I am sure we could make it work if we had to. I kinda like the distinction of "observable instances are the authority and work in the pure, nice way that MST does", and "readonly instances are bastardized to all hell to make them as performant as possible, which is ok, because they are readonly and won't ever be written back to disk".