Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Proof of concept, microtasks to batch batches. #2311

Closed
wants to merge 3 commits into from
Closed

Proof of concept, microtasks to batch batches. #2311

wants to merge 3 commits into from

Conversation

sliftist
Copy link

@sliftist sliftist commented Mar 13, 2020 •
edited
Loading

(This isn't a serious pull request, just a proof of concept).

mobx uses actions to batch observable changes, but I was thinking it could have an option to use microtasks to batch changes, at least when changes are made outside of an action.

I tried it out, and it seems to work, I deployed a highly experimental build at slift/mobx (yarn add sliftist/mobx#0548ddc4179aa87452fd46f621ebf114f854b7da), and am going to start using that branch in projects to get additional testing.

My implementation isn't meant to be a serious implementation, it basically just does this:

public reportChanged() {
    startBatch()
    propagateChanged(this)
    endBatch()
+   if (globalState.queueReportChangedEndBatchAsMicrotask) {
+       Promise.resolve().then(() => {
+           endBatch()
+       })
+   } else {
        endBatch()
+   }
}

If this is something mobx is interested in pursuing, I would need to make changes to ensure microtasks are only used outside of actions, instead of in all reportChanged calls.

Batching changes via a microtask can yield a large performance increase. For example:

@observable lookup = {};
constructor() {
    this.runLoop();
    autorun(this.reactionHandler.bind(this));
}
async runLoop() {
    while(true) {
        await new Promise((resolve) => setTimeout(resolve, 1000));
        for(let key in this.lookup) {
            this.lookup[key]++;
        }
        this.lookup[Date.now()] = 0;
    }
}
reactionHandler() {
    let sum = 0;
    for(let key in this.lookup) {
        sum += this.lookup[key];
    }
    console.log("Keys", Object.keys(this.lookup).length, "Sum", sum);
}

This case scales O(N^2) without batching, due to every lookup change triggering the reactionHandler, but O(N) with batching.

I'm unsure on the full ramifications of this change; if there are parts of mobx that are incompatible with this approach, or if instead it would be a relatively stable change (behind a flag), so guidance would be greatly appreciated!

Related:

@mweststrate
Copy link
Member

mweststrate commented Mar 14, 2020 via email

@mweststrate
Copy link
Member

mweststrate commented Mar 14, 2020 via email

@mweststrate
Copy link
Member

mweststrate commented Mar 14, 2020 via email

@spion
Copy link

spion commented Mar 14, 2020 •
edited
Loading

@mweststrate One of the things I've been thinking about is that strict mode doesn't fully solve the problem. You could have an action in a class, then loop over a list of instances of that class calling their actions. Strict mode will not complain, but reactions will definitely be slow.

Some sort of warning in development mode where > 10 (or N=?) separate actions are detected within one microtask loop would definitely be useful for debugging these cases.

@sliftist
Copy link
Author

@mweststrate , ah that makes sense, keeping side effects in the same call stack is indeed very useful. I didn't realize that was specifically a design principle of mobx, in which case I'll just close this pull request. Thanks!

For anyone that wants this kind of behavior (as in, batching all reactions), the userland utility to accomplish this is:

configure({ reactionScheduler: (callback) => Promise.resolve().then(callback) })

However as @mweststrate mentioned, this will detach the call stack from the original action, and so debugging what line of code triggered a reaction will be much more difficult. Devtools will give the async call stack, so you will be able to find the original action, however the programmatic stack trace (ie, the one you send to the server) from new Error() will only contain the synchronous stack trace.

Also this impacts all actions, not just code that is not inside of an action, so existing actions will begin to trigger reactions differently (as in later).

@spion , I'm not 100% sure, but it appears that nested actions are 'flattened' to some degree. At least in my limited testing if your outer loop is inside of an action (or wrapped with runInAction) side effects are only triggered after the outer most action finishes. That doesn't help the case if the actions are triggered in an independent fashion though.

I agree with @spion that a warning when too many reactions are triggered within a single microtask loop would be very useful.

@sliftist sliftist closed this Mar 14, 2020
@sliftist sliftist mentioned this pull request Mar 14, 2020
Closed
4 tasks
@sliftist
Copy link
Author

I created a pull request to add a warning if too many reactions are triggered with a microtask here: #2312 .

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sliftist @mweststrate @spion