Experiment to find the root cause of the bootlenecks

[echarles]

Experiment to find the root cause of the bootlenecks in https://github.com/jupyterlab/benchmarks/tree/master/experiments

[echarles]

We had a meeting with @ellisonbg @jasongrout and @goanpeca to review the codemirror issues and the resize behavior in jlab/lumino. The following scenario tries to describe the current behavior to better understand what is going on based on the simple experiment of https://github.com/jupyterlab/benchmarks/tree/ee123a929d7130b8e9148ad70efcf15d0462b296/experiments/jlab-cell-boxpanel (1000 jlab cells wrapped into a lumino boxpanel)

From the profiling, we see the repetition on each cell add of the following patttern with force layout which bring additional latency compared to 1000 jlab cells not wrapped into a lumino boxpanel)

We the inspect the methods that generate a force layout. The root is at the lumino processMessage method which is triggered by the following chain (the very first source is an update-request event)

We zoom at the processMessage method

and we move on at the onResize located in @jupyterlab/codemirror/lib/widget.js

We go then into @jupyterlab/codemirror/lib/editor.js where resizeToFit is invoked and considers there is a focus and calls refresh

The refresh in @jupyterlab/codemirror/lib/editor.js finally triggers the CodeMirror refresh method that will end up in a forced layout.

It looks like the refresh in @jupyterlab/codemirror/lib/editor.js is also triggered from time to time further to an update-request triggered by a Widget.fit which add more forced layouts

[echarles]

So prolly something could be done on jlab cells level rather than lumino.

@ellisonbg has suggested to test with simple DIVs in a Lumino BoxPanel, which has been done in https://github.com/jupyterlab/benchmarks/tree/b9695a8d8b948e01dc56e43a6099fe8767ba0709/experiments/lumino-div-boxpanel

That experiment does not show any latency when adding div, which tends to confirm the above outcomes: The forced refresh is triggered by jlab cells.

[echarles]

A bit more experiments on the jlab side.

1. I have disabled in codeeditor/src/widget.ts

   protected onUpdateRequest(msg: Message): void {
+    /*
     if (this.isVisible) {
       this._hasRefreshedSinceAttach = true;
+      this.editor.refresh();  
     }
+    */
   }

This lowers down (without functional impact) from 15 seconds to 10 seconds to load a 50 codecells notebook, so 5 seconds faster, with the following pattern

You can see 3 parts: the fromJson, the setOption, and the third part which was before much larger due to forcedLayout. So we are good on part 3, let's see about part 2 which gives forcedLayout in every setOption.

We have many places in codemirror/src/editor.ts where cm.setOption is called. I have removed them, and we go from 10 second to 5 seconds - In that case, the cells need to be clicked to show the content.

So

• Removing the call to refresh: 15s -> 10s without impact on user side.
• Disabling the setOption: 10s -> 5s - I have looked at the codemirror api but there is no way to run batch options setting which could help

[goanpeca]

Disabling the setOption: 10s -> 5s - I have looked at the codemirror api but there is no way to run batch options setting which could help

@echarles would this be an opportunity to wrap all the setOption inside an operation ?

[echarles]

@echarles would this be an opportunity to wrap all the setOption inside an operation ?

That is a great idea. I was struggling with 2 issues with all those setOption:

1. There is no CodeMirror API to batch the setting of the options => You idea solve it.
2. The request to set options is spread all across the codemirror/src/editor.ts and I am not sure if there is an easy way to concentrate them at one place, not sure either is a method is exposed publically and can be call by an external component to set an option. If we can not make it perfect, we could at least concentrate as much as possible in the editor.ts and use the CodeMirror operation to barch multiple option settings.

[goanpeca]

If we can not make it perfect, we could at least concentrate as much as possible in the editor.ts and use the CodeMirror operation to barch multiple option settings.

Yes, that is what I thought, so concentrate as much as possible in a single place. Maybe create a new setOptions method and try to use that instead (internally this method would use an operation).

Also it feels like most of these options work "on initialization" (on initi method but, on creating a new code editor) so it "should" be possible to concentrate them in a single place.

[goanpeca]

not sure either is a method is exposed publically and can be call by an external component to set an option.

Well, if there is the method could be updated to take an {}? to set multiple options at once also?

[goanpeca]

So we need maybe to turn this

  export function setOption<K extends keyof CodeMirrorEditor.IConfig>(
    editor: CodeMirror.Editor,
    option: K,
    value: CodeMirrorEditor.IConfig[K],
    config: CodeMirrorEditor.IConfig
  ):

Into something like setOptions and try to remove as much of the setOption calls as possible (and centralize them)

  export function setOptions(
    editor: CodeMirror.Editor,
    options: ICodeMirrorOptions,
    config: CodeMirrorEditor.IConfig
  ):

[echarles]

Into something like setOptions and try to remove as much of the setOption calls as possible (and centralize them)

Thx @goanpeca Would be great to see your how you can progress with this. Maybe @jasongrout can give advice and background on this?