Holding off on this to work on#3499, which is a prerequisite.

Current design question:

The InstanceTrackers now provide an IDocumentWidget for document widgets. This means that often people have to interact with the.content attribute. Should we typically provide access to various content attributes at the top-level document widget? For example, in the file editor, people will typically now dowidget.content.editor to get to the editor, whereas before they just didwidget.editor.

I can explain more - the above may be more a short-hand note to myself about what to bring up in a design discussion.

@blink1073,@afshin - do you remember why we let people change the context of an existing notebook panel?

Is that something we should be careful about preserving, or is it something we can move into the constructor and make readonly?

Edit: it seems at some point we had design discussions about passing themodel into the constructor, versus setting the model after constructing the object, but I don't remember the context being part of that discussion

Any ideas? I still can't figure out what is going on.

I seem to get some contradictory results. It seems that when I remove the reveal promise, subjectively things are fast again (I should test this again). However, when profiling, it seems that the extra time is spent in a browser layout, laying out thousands of nodes (e.g., all of the internal codemirror text nodes), which I think is maybe triggered by the phosphor BoxLayout resizing an absolute-sized codemirror div - that's my current working hypothesis.

For some hard numbers, with a 7289859 byte single-line main.*.js.map file, I'm seeing, on Chrome 66 on macOS:

Master (609e4fb): 20s to initially load, 20s to activate tab

This branch (6749792): 80s to initially load, 20s to activate tab

Interestingly, I am seeing similar numbers to you on Chrome, but Firefox 60 ismuch faster on master. All with loading the file on Ubuntu 16.10:

Chrome 64 (master/context): 20s/60s
Firefox 60 (master/context) 6s/80s

With this patch on this branch (6749792), I get about 23s on load, 20s on tab activation:

diff --git a/packages/docregistry/src/default.ts b/packages/docregistry/src/default.tsindex c54c7dd4b..dfe4e3fc7 100644--- a/packages/docregistry/src/default.ts+++ b/packages/docregistry/src/default.ts@@ -420,7 +420,7 @@ class DocumentWidget<T extends Widget = Widget, U extends DocumentRegistry.IMode   constructor(options: DocumentWidget.IOptions<T, U>) {      // Include the context ready promise in the widget reveal promise-    options.reveal = Promise.all([options.reveal, options.context.ready]);+    // options.reveal = Promise.all([options.reveal, options.context.ready]);     super(options);      this.context = options.context;

I can confirm I also see the speedup when commenting out that line.

With this patch instead (on6749792), I'm also seeing the faster 20s load time:

diff --git a/packages/docregistry/src/default.ts b/packages/docregistry/src/default.tsindex c54c7dd4b..91f96bb06 100644--- a/packages/docregistry/src/default.ts+++ b/packages/docregistry/src/default.ts@@ -420,7 +420,8 @@ class DocumentWidget<T extends Widget = Widget, U extends DocumentRegistry.IMode   constructor(options: DocumentWidget.IOptions<T, U>) {      // Include the context ready promise in the widget reveal promise-    options.reveal = Promise.all([options.reveal, options.context.ready]);+    // options.reveal = Promise.all([options.reveal, options.context.ready]);+    options.reveal = Promise.resolve();     super(options);      this.context = options.context;

Got to get you on Firefox, with these sweet 6 second loading times :)

I was thinking that having any promise was tripping the longer load time, but even when we do take the asynchronous route in the main area widget constructor, we still can get fast load times.

Iam on ff - just when I was profiling I started using Chrome since it has way better debugging tools in general.

(and the vast majority of the populace uses Chrome, so we should make sure it works there too :).

Define the following utility functions in that docregistry/src/default.ts file:

/** * Return a promise that resolves in the given milliseconds with the given value. */exportfunctionsleep<T>(milliseconds:number=0,value?:T):Promise<T>{returnnewPromise((resolve,reject)=>{setTimeout(()=>{resolve(value);},milliseconds);});}/** * Return a promise that resolves in the next animatino frame with the particular value. */exportfunctionmoment<T>(value?:T):Promise<T>{returnnewPromise((resolve,reject)=>{requestAnimationFrame(()=>{resolve(value);});});}

Then makingoptions.reveal on line 420ishsleep(0) has a short load time,moment() has a short load time, andsleep(500) has a long load time.

Current working hypothesis - if the editor value is set before the reveal promise is resolved, things are slow. If the editor value is set after the reveal promise is resolved, things are fast.

I think that CodeMirror may be having layout issues if the editor is not actually attached to the DOM. If I add the content widgetbefore thereveal promise resolves, everything looks okay. The spinner still shows up, since it's CSS makes it an overlay, and it loads much faster.

That is to say, removelayout.addWidget(content) from thereveal promise handlers, and add it to the synchronous part of the constructor. Everything else can stay the same.

That was one of the next couple of things I was going to try. Thanks for the tip to shortcut the experiments!

More experiments...

(a) If I attach the content and toolbar, it takes up some of the space and the spinner is pushed down.
(b) If I attach the content, but then hide it until the reveal promise resolves, it goes back to taking a long time.

In some profiling, it seemed that the content focus was triggering a layout that was taking a long time. However, eliminating the focus call in the main area widget constructor on top of (b) still took a long time.

Hmmm, sounds like another div may be necessary.

I reverted back to using the spinner as just a DOM node, rather than treating it like a phosphor widget. This is a bit kludgy since you aren't supposed to mess around with a BoxLayout DOM node, but it works for now, and we can investigate more later.

That works for me. We could use aStackedLayout for the same purpose later, yes?

What prompted you to change your mind on whethercontent could benull?

Change in content being null was from it being easier to write the check, it being our common pattern, and thinking more about how disposing the content is linked to disposing the main widget.

Merging, as the appveyor issues are unrelated. Thanks@jasongrout!