Derive computed states only when required for delayed Reactions #3819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
馃 Changeset detectedLatest commit: 3e56277 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
taj-p
commented
Jan 24, 2024
packages/mobx/src/core/reaction.ts
Outdated
Comment on lines
97
to
106
| if (!this.scheduledRunReaction_) { | ||
| this.scheduledRunReaction_ = true | ||
| this.scheduler(() => { | ||
| this.scheduledRunReaction_ = false | ||
| this.runReactionImpl() | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| private runReactionImpl() { |
There was a problem hiding this comment.
These are the core changes - Reaction dictates when it should runReactionImpl based on the passed in scheduler.
taj-p
commented
Jan 24, 2024
Comment on lines
+88
to
+133
| test("autorun re-calculates computed states only when required", function (done) { | ||
| let autoruns = 0 | ||
| let computedCalcs = 0 | ||
|
|
||
| const o = mobx.observable.box(0) | ||
| const c = mobx.computed(() => { | ||
| computedCalcs++ | ||
| return o.get() | ||
| }) | ||
|
|
||
| mobx.autorun( | ||
| () => { | ||
| c.get() | ||
| autoruns++ | ||
| }, | ||
| { delay: 300 } | ||
| ) | ||
| expect(computedCalcs).toBe(0) | ||
|
|
||
| setTimeout(() => { | ||
| // We expect the first run to run the computed | ||
| expect(autoruns).toBe(1) | ||
| expect(computedCalcs).toBe(1) | ||
| }, 400) | ||
|
|
||
| // We shouldn't eagerly calculate the state before the autorun runs because | ||
| // the state can be mutated again before `delay` expires - if that happens, a | ||
| // re-calculation here would be wasted. | ||
| setTimeout(() => { | ||
| o.set(1) | ||
| expect(computedCalcs).toBe(1) | ||
| }, 800) | ||
|
|
||
| // This will re-invalidate the state so we'll need to recalculate | ||
| // again when autorun runs (when `delay` expires). | ||
| setTimeout(() => { | ||
| o.set(3) | ||
| expect(computedCalcs).toBe(1) | ||
| }, 900) | ||
|
|
||
| setTimeout(() => { | ||
| expect(autoruns).toBe(2) | ||
| expect(computedCalcs).toBe(2) | ||
| done() | ||
| }, 1600) | ||
| }) |
There was a problem hiding this comment.
Thanks @realyze - your reproduction helped craft this test
There was a problem hiding this comment.
For reference to reviewers: the same test fails on the latest main (where you can see the unnecessary derivations being computed):
taj-p
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Derive computed states only when required for delayed Reactions
Fixes #3724
Context
We use MobX with React. Due to the above issue, we are triggering unnecessary derivations for computed values (and contexts) for
autoruns withdelayor custom schedulers. This means that, at times, the user presses a button on screen and a render is scheduled, but, before the browser paints, it has to compute some derivation that is not required for the render.Sometimes our derivations are expensive, so we really rely on deferring the (re)calculation of computed values until a delayed
autorunneeds to read them. Moreover, without these changes, we trigger unnescessary (re)calculations prior to when a delayedautorunruns.Changes
The core change of this PR is to lift the "scheduler" from the
autorunandreactionintoReaction. The rationale for this change is (I haven't contributed to this library before, so please scrutinise these reasons closely 馃檹 ):It is within
ReactionthatshouldComputeruns. I.e., theReactionitself determines whether its been invalidated and needs to run itsonInvalidate, so, I reasoned that it should also be responsible for scheduling that invalidation.mobx/packages/mobx/src/core/reaction.ts
Line 98 in df7e1eb
Benefits
Again, I am a MobX noob, so these are the perceived benefits from a novice's POV:
delay) and only deriving them when needed.autorunandreactionAPIs appear a bit simpler since we've consolidated the scheduling logic toReaction.Disadvantages
Are we moving logic to Reaction that shouldn't be moved there? I think it makes sense for a Reaction to schedule its own
onInvalidation, but I don't have the full context of the library.Code change checklist
[ ] Updated/docs. For new functionality, at leastAPI.mdshould be updatedyarn mobx test:performance)cc @urugator