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

Prevent ModelObserver update backlogs #541

Merged
merged 9 commits into from Feb 22, 2017

Conversation

Projects
None yet
3 participants
@BinaryMuse
Member

BinaryMuse commented Feb 19, 2017

The ModelObserver is used as a pseudo-declarative way to watch for models that implement onDidUpdate to change and then take some action (usually fetching new data from the underlying model). This pattern is made more fully declarative via the ObserveModel decorator.

Take the following scenario:

class Model {
  // ...

  getSomeData() {
    return new Promise(...) // takes some time to resolve
  }
}

const mo = new ModelObserver({
  fetchData: model => model.getSomeData(),
  didUpdate: () => this.refreshSomeViewOrSomething(),
});
mo.setActiveModel(new Model());

As written currently, the following scenario could take place:

┌──────────────────┐                          ┌─────────────────────┐
│ 1. onDidUpdate() │                          │  6. data discarded  │
└───┬──────────────┘                          └───┬─────────────────┘
    │                                             │
┌───┼─────────────────────────────────────────────┼──────────────────┐
│   ▼                                             ▼                  │
│  ╔══════════════════════════════════════════════╗                  │
│  ║            2. getSomeData() (3s)             ║                  │
│  ╚══════════════════════════════════════════════╝                  │
│                                                                    │
│      ╔════════════════════════════╗                                │
│      ║   4. getSomeData() (2s)    ║                                │
│      ╚════════════════════════════╝                                │
│       ▲                           ▲                                │
└───────┼───────────────────────────┼────────────────────────────────┘
        │                           │
        │                           │
     ┌──┴───────────────┐       ┌───┴──────────────┐
     │ 3. onDidUpdate() │       │   5. data set    │
     └──────────────────┘       └──────────────────┘
  1. Something triggers an update in the model, which triggers an onDidUpdate callback in the observer.
  2. The observer calls getSomeData on the model. This takes 3 seconds to complete.
  3. In the meantime, another operation triggers an update in the model. The onDidUpdate callback fires again.
  4. The observer calls getSomeData again. This time it takes 2 seconds to complete.
  5. The second getSomeData callback completes, and since that promise is associated with the latest fetch, it's set as the "current" data.
  6. The first getSomeData promise resolves a second later, but since it's associated with an older fetch, the data is considered stale and is thrown away.

Normally, this is perfect — we're able to run multiple fetches concurrently, and only the latest one is actually used, rendering updates as quickly as possible.

However, this triggers a problem when fetching data from the Repository model, since reads are queued (up to a certain point controlled by a parallelism limit). In that case, every read ends up piling on the queue:

 Read queue:
┌────────────────────────────────────┐
│                                    │
│ ┌─────┐                            │
│ └─────┘                            │
│   ┌─────┐                          │
│   └─────┘                          │
│          ┌─────┐                   │
│          └─────┘                   │
│                                    │
│                                    │
│                                    │
│                  ┌─────┐           │
│                  └─────┘           │
│                    ┌─────┐         │
│                    └─────┘         │
│                           ┌─────┐  │
│                           └─────┘  │
└─┬──────────────┬┬────────────────┬─┘
  │ Queued reads ││ Queued reads   │
  │  from first  ││  from second   │
  │ getSomeData()││ getSomeData()  │
  │              ││                │
  └──────────────┘└────────────────┘

In this case, the reads are queued such that the second getSomeData() is forced to wait the rest of the 3 seconds from the initial fetch before it can start it's own 2 seconds worth of fetching, making the time profile look more like this:

┌──────────────────┐                          ┌─────────────────────┐
│ 1. onDidUpdate() │                          │  5. data discarded  │
└───┬──────────────┘                          └───┬─────────────────┘
    │                                             │
┌───┼─────────────────────────────────────────────┼───────────────────────────────┐
│   ▼                                             ▼                               │
│  ╔══════════════════════════════════════════════╗                               │
│  ║            2. getSomeData() (3s)             ║                               │
│  ╚══════════════════════════════════════════════╝                               │
│                                                                                 │
│      ╔══════════════════════════════════════════╦═════════════════════════════╗ │
│      ║     4. getSomeData() (queued reads)      ║        actual reads         ║ │
│      ╚══════════════════════════════════════════╩═════════════════════════════╝ │
│       ▲                                                                       ▲ │
└───────┼───────────────────────────────────────────────────────────────────────┼─┘
        │                                                                       │
        │                                                                       │
     ┌──┴───────────────┐                                      ┌────────────────┴─┐
     │ 3. onDidUpdate() │                                      │   6. data set    │
     └──────────────────┘                                      └──────────────────┘

So not only does the update feel like it's way slower, because we don't set any data until the latest fetch has all its reads finally executed, but multiple triggers of onDidUpdate() could cause many reads to be placed into the queue, thus causing a huge delay in the actual operations that will ultimately actually be used. This is an actual problem for us, as nsfw triggers updates in batches; in large repos, we've observed as many as 12 discrete events triggering model updates in just over a second, causing the Git operation queue to be saturated for a good 10 or 15 seconds before any UI updates occur.


This PR changes the ModelObserver semantics such that:

  1. If a model fetch is pending when another update is requested, that second update is queued to start after the other is finished.
  2. If a model fetch is pending when another update is requested and we've already queued another update, we discard it.
  3. Any time a fetch finishes, we set it as the current data, even if another update is pending.
  4. After a fetch finishes, run any pending update.

That means we lose concurrent fetches, but we gain the ability to have "partial" updates when a model has only partially updated. This would change the graph to be more like this:

┌──────────────────┐                          ┌────────────────────────────────┐
│ 1. onDidUpdate() │                          │ 4. data is SET (not discarded) │
└───┬──────────────┘                          └───┬────────────────────────────┘
    │                                             │
┌───┼─────────────────────────────────────────────┼───────────────────────────────┐
│   ▼                                             ▼                               │
│  ╔══════════════════════════════════════════════╗                               │
│  ║            2. getSomeData() (3s)             ║                               │
│  ╚══════════════════════════════════════════════╝                               │
│                                                                                 │
│       ╔══════════════════════════════════════════╦════════════════════════════╗ │
│       ║             (pending update)             ║   5. getSomeData() (2s)    ║ │
│       ╚══════════════════════════════════════════╩════════════════════════════╝ │
│       ▲                                                                       ▲ │
└───────┼───────────────────────────────────────────────────────────────────────┼─┘
        │                                                                       │
        │                                                                       │
     ┌──┴───────────────┐                                      ┌────────────────┴─┐
     │ 3. onDidUpdate() │                                      │   6. data set    │
     └──────────────────┘                                      └──────────────────┘

The additional benefit is that no matter how many onDidUpdate callbacks are triggered during the original 3-second fetch, there will only be one additional fetch run after it completes; for us, this results in a Git operation queue that has many fewer operations queued.

We also considered throttling nsfw events at the watcher level, but feel this is ultimately a better experience since partial updates are rendered to the UI as soon as possible (much like unix commands that stream text to a terminal as things update). We've tested this with a large merge conflict in a fairly large repo (github/github) and it feels much better, especially with other changes to the read queue that are still in progress (to better speed up and parallelize the queue).

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Feb 20, 2017

Member

@kuychaco Do you mind looking over above and making sure I didn't miss anything?

Member

BinaryMuse commented Feb 20, 2017

@kuychaco Do you mind looking over above and making sure I didn't miss anything?

@BinaryMuse BinaryMuse self-assigned this Feb 20, 2017

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Feb 20, 2017

Member

The tests have been brought up to date for the new behavior.

@kuychaco, I'd like to remove the await from await this.didUpdate(model) in ModelObserver#_refreshModelData. I don't think the timing of the next data fetch should depend on the actions performed in the user-provided didUpdate. It's possible the user will return promises for UI updates or other things from it, and I think we should leave the timing of the side effects up to the user and move on with the next pending fetch (if there is one) immediately.

If you're in favor, I'll make the change; it breaks a lot of other tests (presumably where we're returning etch.update(this) from inside it) but they can likely be fixed without too much trouble.

Member

BinaryMuse commented Feb 20, 2017

The tests have been brought up to date for the new behavior.

@kuychaco, I'd like to remove the await from await this.didUpdate(model) in ModelObserver#_refreshModelData. I don't think the timing of the next data fetch should depend on the actions performed in the user-provided didUpdate. It's possible the user will return promises for UI updates or other things from it, and I think we should leave the timing of the side effects up to the user and move on with the next pending fetch (if there is one) immediately.

If you're in favor, I'll make the change; it breaks a lot of other tests (presumably where we're returning etch.update(this) from inside it) but they can likely be fixed without too much trouble.

@BinaryMuse BinaryMuse requested a review from kuychaco Feb 20, 2017

Show outdated Hide outdated test/models/model-observer.test.js
}
fetchA() {
this.fetchACallCount++;

This comment has been minimized.

@kuychaco

kuychaco Feb 21, 2017

Member

🔥

Show outdated Hide outdated test/models/model-observer.test.js
}
fetchB() {
this.fetchBCallCount++;

This comment has been minimized.

@kuychaco

kuychaco Feb 21, 2017

Member

🔥

@kuychaco

Looks good :shipit:

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Feb 22, 2017

Member

Also, @BinaryMuse, your ASCII-art game is 🤘

Member

smashwilson commented Feb 22, 2017

Also, @BinaryMuse, your ASCII-art game is 🤘

@BinaryMuse BinaryMuse merged commit bd75273 into master Feb 22, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
schema/push Schema pull request already exists
Details

@BinaryMuse BinaryMuse deleted the mkt-ku-prevent-model-observer-backlog branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment