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

@snapshottedView #69

Closed
wants to merge 2 commits into from
Closed

@snapshottedView #69

wants to merge 2 commits into from

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented Nov 9, 2023

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:

@register
class ViewExample extends ClassModel({ key: types.identifier, name: types.string }) {
  @snapshottedView()
  get slug() {
    return this.name.toLowerCase().replace(/ /g, "-");
  }

  @action
  setName(name: string) {
    this.name = name;
  }
}

And I change some stuff about it, the snapshot will contain the computed slug in it::

const instance = ViewExample.create({ key: "123", name: "Foous Barius"})
instance.setName("A different name");
const snap = getSnapshot(instance); // => { key: "123", name: "A different name", slug: "a-different-name"}

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:

const readOnlyInstance = ViewExample.createReadOnly(snap);
readOnlyInstance.slug // => "a-different-name", much faster

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 wont ever be written back to disk".

Right after observable instance creation, should the snapshot have the snapshotted views in it?

With the following class model:

@register
class ViewExample extends ClassModel({ key: types.identifier, name: types.string }) {
  @snapshottedView()
  get slug() {
    return this.name.toLowerCase().replace(/ /g, "-");
  }

  @action
  setName(name: string) {
    this.name = name;
  }
}

I can do this to ask for the initial snapshot of the instance:

const instance = ViewExample.create({ key: "123", name: "Foous Barius"})
getSnapshot(instance);

Before snapshotted views ever existed, this pattern could be super performant and just return exactly the input snapshot. Now though, we could say that all snapshots alwas have the value of snapshotted views in them, so that initial getSnapshot call needs to execute the snapshotted views in order to get values for the snapshot. We could say that "if a view hasn't been touched, it won't be in the snapshot", such that snapshots only sometimes have the snapshotted views in them. This would work technically, but I think defeat our purposes Gadget side. Many of our persisters just store getSnapshot(someInstance) in a jsonb column, and if MQT doesn't feed in the snapshotted views into that column, the ROSR won't ever get them to improve performance. So, I think we should make the rule "all snapshots have snapshotted views in them" for simplicity and predictability.

Gadget side PR showing how it might be used: https://github.com/gadget-inc/gadget/pull/8902

mobx-state-tree bugs:

@airhorns airhorns force-pushed the cached-view branch 4 times, most recently from 1766986 to d7c7d0b Compare November 9, 2023 17:59
@airhorns airhorns changed the title cached view @cachedView Nov 9, 2023
@gadget-inc gadget-inc deleted a comment from codspeed-hq bot Nov 13, 2023
Copy link

codspeed-hq bot commented Jun 3, 2024

CodSpeed Performance Report

Merging #69 will improve performances by 10.07%

Comparing cached-view (5017ec1) with main (5daccbe)

Summary

⚡ 11 improvements

Benchmarks breakdown

Benchmark main cached-view Change
instantiating deep references 115.2 µs 89.1 µs +29.34%
instantiating one reference 50.6 µs 39.1 µs +29.32%
instantiating a large union 83.8 µs 60.7 µs +38.03%
mobx-quick-tree ClassModel 53.2 µs 40.4 µs +31.71%
instantiating a diverse root 74.1 µs 54.6 µs +35.7%
instantiating a large root 3.7 ms 2.5 ms +48.83%
instantiating a small root 29.3 µs 21.6 µs +35.75%
accessing memoized getter properties of a class model 14.7 µs 11.4 µs +29.9%
accessing memoized null property of a class model 11.4 µs 10.1 µs +13.35%
accessing unmemoized getter properties of a class model 19.7 µs 15.4 µs +27.77%
accessing unmemoized null property of a class model 13.5 µs 12.2 µs +10.07%

@airhorns airhorns force-pushed the cached-view branch 3 times, most recently from 855ab3a to 755f342 Compare June 3, 2024 20:56
@airhorns airhorns changed the title @cachedView @snapshottedView Jun 4, 2024
@airhorns airhorns force-pushed the cached-view branch 4 times, most recently from 0eae80c to e91d1a6 Compare June 5, 2024 22:33
@danroberts
Copy link

I guess my thought here is if we're going to go down this pathway, why not go to full serialization of the root's values? Partially to me, the main benefit of using the ROSR and MST roots over something like a boring, manual serialization into intermediate data representations was that if we changed the business logic of one of the views, we were guaranteed to compute the same results across all the different root environments.

I wonder if this approach would introduce new complexities to manage when we make changes to the mst representations. For example, I've been finding it a bit challenging to wrap my head around making changes to our existing mst structures because they are cached at different levels, and any changes have to be managed around deploy time data structure consistency.

In this case, a code only change to the way that some of these views are computed might end up resulting it different results if run in a ROSR or a FSER -- which may not be intuitive to the developer making the change.

I wonder if there is not some effort and complexity to be saved in the long term if we moved to a more explicit serialization into a runtime data structure which could be version controlled more easily and separately from runtime code.

@scott-rc scott-rc mentioned this pull request Jun 18, 2024
@airhorns airhorns closed this Aug 7, 2024
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.

2 participants