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

Add computed.shallow annotation #2986

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Jun 15, 2021

In our (Canva) codebase, I've seen several places where computed.shallow would be useful, where we are currently using either computed.struct (unnecessarily) or a custom comparer function which is much longer to write. So I think it would be useful to add a computed.shallow parallel to observable.shallow.

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (npm run perf)

@changeset-bot
Copy link

changeset-bot bot commented Jun 15, 2021

🦋 Changeset detected

Latest commit: ba43d3f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@urugator
Copy link
Collaborator

@urugator
Copy link
Collaborator

Just in case you're not aware, we do export shallow comparer, so you can do @computed({ equals: comparer.shallow })

@upsuper
Copy link
Contributor Author

upsuper commented Jun 17, 2021

I don't quite agree with the reasoning there. It's been mentioned by the author of that issue as well that an O(n) comparison is not bad given that it already takes O(n) to generate the collection, and if a downstream is triggered, it will likely also take O(n) to handle the change.

On the other hand, I believe the existence of @computed.struct actually makes things worse, because when one wants to compare collections, it's more likely that they would just jump to @computed.struct even if it's unnecessarily more expensive, rather than writing the much longer form of @computed({ equals: comparer.shallow }). So unless you are actively discouraging using @computed.struct (e.g. marking it deprecated and suggesting user to use @computed({ equals: comparer.structural }) instead), I think it's better to also add a shorthand for the middle ground.

@urugator
Copy link
Collaborator

urugator commented Jun 17, 2021

it will likely also take O(n) to handle the change.

I assume you mean "at least O(n)"? ... otherwise it takes O(n) (compare), to prevent O(n) (handle the change), so it never optimizes anything.

Personally I would vote for deprecating @computed.struct. Since these are optimizations, require some thought and shouldn't be needed on daily basis, the "long" version seems appropriate. It also simplifies the API - one way to do one thing.

@upsuper
Copy link
Contributor Author

upsuper commented Jun 17, 2021

I assume you mean "at least O(n)"? ... otherwise it takes O(n) (compare), to prevent O(n) (handle the change), so it never optimizes anything.

Well not all O(n)s are equal. A shallow comparison should be pretty cheap, but there are more expensive O(n)s like if you rerender a list in a React component, which then triggers reconciliation. Technically it will still be O(n) if nothing changes, but it's a much more expensive O(n) than the comparison.

@urugator
Copy link
Collaborator

Well not all O(n)s are equal.

Sure, just saying that the O(n) (handle the change) must be more expensive than comparison and if I follow correctly, you claim that this is usually the case, which I agree, but imo in practice it must be significantly more expensive to be effective.

which then triggers reconciliation

Be careful so you're not simply replacing comparison of elements performed by react by a comparison of array items performed by mobx. There is no point in preventing react's reconciliation by doing own reconciliation, unless there is an expensive step in the middle (render function itself).

@upsuper
Copy link
Contributor Author

upsuper commented Jun 17, 2021

React's reconciliation is... reconciliation, mobx's shallow comparison is not. Reconciliation isn't just a shallow comparison, it needs to compare constructor and props for each node as well on top of finding the match, because they are all new objects, and don't forget that the render function is generating all these new objects. Shallow comparison on the other hand can rely on object reference equality check which should be much cheaper.

Also I would argue it's a pretty common case where changes to a collection almost always come with a size change (new item being added and removed rarely happen simultaneously), in those cases a shallow comparison on unequal case is O(1), and on equal case is a cheap O(n) to avoid more expensive process.

@mweststrate
Copy link
Member

Also I would argue it's a pretty common case where changes to a collection almost always come with a size change

@upsuper here you touch on an important point; the performance impact isn't just determined by the expensiveness of the shallow comparison, but also by the amount of 'cache hits'. I think for the vast majority of computed expressions, the shallow comparison will never hit 'unchanged', because typically the computed wouldn't have fired in the first case if none of its own deps weren't actually changed:

Take the following example:

class Vector {
  @observable x = 1
  @observable y = 2

  @computed get coords() {
     return { x: this.x, y: this.y }
   }
}

It might be tempting to put a computed.shallow here on coords, but it is completely pointless, because in any case where shallowly the same object would be produced, the coords computed wouldn't be fired in the first place! Either you assign the same value to x/y, and the coords computed won't fire at all, or you assigned a different value and the shallow comparision will always yield false.

A computed.shallow will only have any benefit if different inputs can still lead to the same output, for example

// this shallow helps, as `x` 71 and 72 will produce structurally the same object in the end with different inputs:
@computed.shallow get coords() {
  return { x: Math.floor(this.x / 10), y: this.y
}

That being said I'm fine adding this, even for consistency. Would you mind expanding the docs in this PR that those decorators (similar reasoning applies to .struct) only add any benefits in specific cases, but not in general?

@hearnden
Copy link
Contributor

hearnden commented Nov 1, 2023

Any progress on this PR? I think it's great!

A computed.shallow will only have any benefit if different inputs can still lead to the same output

Absolutely. It needs to be some kind of a reduction computation, not just returning the same data in a different shape.

....and I'm not so sure that's rare in practice. In our codebase at least, it happens all the time.

We have lots of cases in our codebase of classes that contain observable mutable data that also expose computed properties that reveal structured reductions of that data (e.g., combos of max, min, filter, ...). The recomputation of those reductions is O(n), yes, but when most mutations don't change the reduced value and the comparator is able to detect that the reduced value hasn't changed, then it doesn't trigger further recomputations, and that's the win.

Considering the example above with Vector, but changing it from one that "is coordinates" to one that "has coordinates", motivates the .shallow case as a natural analogue of the .struct case.

type HasCoords = { x: number, y: number };

/** A collection of things with coordinates. */
class ElementCollection<T extends HasCoords> {
  @observable.ref
  es: readonly T[] = [];

  add(e: T) {
    this.es.push(e);
  }  

  /** The bounding box covering the elements. */
  @computed.struct  // <-- sensible use of .struct
  get boundingBox(): { minX: number, maxX: number, minY: number, maxY: number } {
    ...
  }

  /** Elements that lie on a bounding-box boundary. */
  @computed({ equals: comparer.shallow }) // <-- crying out for a computed.shallow shorthand!
  get extrema(): readonly T[] {
    const { minX, maxX, minY, maxY } = this.boundingBox;
    return this.es.filter(e => e.x === minX || e.x === maxX || e.y === minY || e.y == maxY);
  }

  @computed  // <-- this is something that doesn't make sense to .struct or .shallow on, as you point out
  get sameDataDifferentShape() {
    return { copy: [...this.es], json: JSON.stringify(this.es) };
  }
}

We use @computed({ equals: comparer.shallow }) and @computed({ equals: comparer.struct }) (now computed.struct) quite a lot, just like we use @observable.shallow and @observable.struct quite a lot. It would be great to have this final shorthand to complete the set!

@mweststrate
Copy link
Member

Sorry, have been focused on other priorities. Let me wrap up #3638 first, and check if it cleanly applies afterwards.

In the mean time for anyone reading along, note that decorators are just higher order fns, so feel free to work around by defining your own utility should work:

const computedShallow = computed({ equals: comparer.shallow })

// later

class X {
  @computedShallow
  get myComputation() { ...
 }

@mweststrate
Copy link
Member

I think we could revisit this now that decorators have landed. Is there still a need / interest for this feature @upsuper?

@upsuper
Copy link
Contributor Author

upsuper commented May 30, 2024

Sorry I have been quite busy lately, and almost forgot about this one. But yes, there is definitely still a need. I'll have a look into it tomorrow.

@upsuper
Copy link
Contributor Author

upsuper commented May 31, 2024

@mweststrate I've rebased the branch to the latest and add some more documentation for computed.shallow. Hope this looks better now.

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.

4 participants