Explore notebook list view impl

[rebornix]

We currently a dozen of layout related issues #169038 and fixing them requires having more patches on the our notebook cell list. Let's see if we can swap out the default list view impl with our own and simplify the layout logic and at the end, reduce our maintenance cost in overall.

[rebornix]

Here is what I learnt so far while exploring our own list view

1. ListView#render can not be nested. When nested, it might lead to unexpected positioning of elements. ListVIew#render will do the initial cell rendering for the viewport, when there is a cell height change happen before ListView#render finishes, we currently request to do layoutNotebookCell in next animation frame.
2. However, ListView#render always comes with a ListView#_rerender, which will read the layout info of all rendered cells and adjust their positions. It means all layoutNotebookCell requests from previous step (ListView#render) are obsolete. When we handle the requests in next frame, we will find that the height === cell.layoutInfo.totalHeight and skip early.
3. In ListView#_rerender, more cells might be rendered, and if their height changes, there won't be another _rerender run. This means, their height adjustment might only be handled in next frame.
4. When we render code cells, we will request for cell text models, which are offered by modelService#resolveTextModel. It's an async operation so even if it resolves immediately, the callbacks are (thenable) are only invoked after the Event Loop clears its current call stack. That's why we are seeing another set of cell layout updates after the list view finishes the viewport rendering completely, as they come pretty late (still in the same AF though).

With these in mind, here are some optimizations we can make to the notebook cell list view:

• If a layoutNotebookCell happens outside of ListView#render, let's run it immediately instead of postponing it to next AF.
• When we are inside ListView#render and cell height changes, add it to a cell height update queue. If the queue is empty after we run ListView#render, no need to call _rerender.
• When we are inside ListVIew#_rerender and cell height changes, add it a cell height update queue. If the queue is not empty after we run ListView#_rerender, request another run of ListView#_rerender.

Future more, we could explore

• Instead of rendering all potentially visible cells first, checking their height, and lastly removing out-of-viewport ones or rendering more cells, we can probably render cells one by one and check their height right after they are rendered.
• Caching more layout state. Currently we only cache total height, so when we are rendering a cell, we accidentally trigger a layout update when we try to render cell outputs, since the output rendered height doesn't come back from webview yet, we read it as 0 and it triggers the shrinking of the cell total height, which invalidates the cached total height. We could try t cache the output height too.

[rebornix]

After shipping #171681 for around two days, even though we didn't see layout issues coming, I ran into NPEs frequently when opening notebooks. Digging this a bit I realize that nesting render into _rerender is not safe. What can happen now is

• render
  • render element
  • element height change, request to change list element height. Since it's in render, we postpone to next animation frame
• _rerender
  • update element height and position
  • render element if there is more space, say we need to render two more elements: element 9 and element 10.
    • render element 9
    • element height change, and since it's not inside render, we request to update element height immediately
    • ListView#updateElementHeight
    • ListView#render
      • this step will try to update element position for previous render range, which includes element 9 and 10.
      • 🐛 when it attempts to update position for element 10 (items[10].row!.domNode), element 10 is not rendered yet, thus there is no domNode -> 🐛 NPE

This made a bit concerned with _rerender() as inside _rerender, it can change list view scrollPosition, before insertElement, there is potentially a chance that we operate on elements that are not rendered yet (as the rendering is nested now). I pushed fix #171866 for notebook for now but ultimately, we should probably have a queue instead of checking whether we are inside a render or _rerender.

[rebornix]

@joaomoreno 🖕 let's discuss this on Monday. I think to make it correct, we probably do need the pendingUpdateQueue concept, as long as we have element height change while rendering the element.

[joaomoreno]

Here are my thoughts after our discussion:

• Here's the simplified description of how dynamic height works in the list:
  • Instead of providing a definite height, users are supposed to return a height estimate in getHeight().
  • The list will measure the real height once the list element is found to "fit in the current viewport".
• Here's the simplified description of how notebooks are using the list:
  • A height estimate is first returned in getHeight()
  • The list measures a wrong height value, because the notebook renderer "bails out" when the list attempts to probe for the actual height. The list expects a definite height value, but it's getting another estimate once more.
  • So, the notebook is forced to remember the editor cells which were "bailed out" and will soon after call updateElementHeight with the real height.

It's still not 100% clear to me why this behavior was implemented in the first place. The list never expected its user to "lie" when it probes for the actual height. From what I can tell, the current implementation is based on the fact that probing for the actual height of a code editor cell is way too expensive, if that cell is eventually not going to fit in the viewport.

Final thoughts:

• The two-step pattern of [(1) get height estimate and (2) measure real height] feels the most simple to me and I'm confident it is the most performant, no matter how many more calls we do around those two steps. The proposals written in this issue go against that pattern. I'd love to be proven wrong.
• I'm sure there are optimizations to be made in the list's current _rerender method and I suggest we focus on that.
• Question: how expensive is [(1) set model + (2) get height] today, in millis?
• Given that the notebook renderer has full control over the code editor's contributions, could we implement a getHeight() function which would return the definite height value for a code editor, without ever rendering it in the DOM? That is: can tackle the "measuring a real code cell height is expensive" problem directly?
  • If the above happens to be a really hard problem, could we instead have a single hidden code editor laying around just used for accurate height measuring? How expensive would that be in millis?

[rebornix]

measure real height] feels the most simple to me and I'm confident it is the most performant, no matter how many more calls we do around those two steps
Question: how expensive is [(1) set model + (2) get height] today, in millis?

Step 2 used to be noticeably slow as at the time we render some outputs in the renderer process, meaning we are checking the offsetHeight of complex absolutely positioned DOM structures. That's the major difference between Notebook Cell and Settings Editor's markdown preview. It can take tens of ms, and setting models on Monaco Editor can take tens of ms, and the frequent "DOM write" and "DOM read" can lead to forced reflow. When they all add up, it can be near 100ms in its worst case. Unfortunately, we can easily experience the worst case when we are scrolling fast, as list elements are being rendered, measured, and then disposed. Failing to squeeze them into 60ms (16Hz) means sluggish scrolling.

Above is the major reason why we skip "renderElement(height: undefined)", you are correct that we are against this pattern: The proposals written in this issue go against that pattern , we want to avoid Step 2.

Back to today's notebook list view rendering, we are very close to have Step 1 only but only facing two challenges:

• failing to measure code cell editor's target height (word wrap, scrollbar)
• monaco editor emits intermediate wrong height (for example, we are seeing unexpected content height change for scroll bar rendering)

could we instead have a single hidden code editor laying around just used for accurate height measuring

I like this idea a lot. It's worth an experiment.

---

Short summary:

• We don't want to check DOM dimensions. All our optimizations are around that.
• If we are smart to have perfect estimation, _rerender or probeHeight is not needed for us.
• If we don't, we would request "updateElementHeight", but it might conflict with on-going render/_rerender. We'd love to see improvement for this. Our proposal in the issue description doesn't have to be the solution.