Prevent ModelObserver update backlogs

[BinaryMuse]

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]

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

[BinaryMuse]

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.

[kuychaco]

🔥

[kuychaco]

🔥

[kuychaco]

Looks good

[smashwilson]

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