Skip to content
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

Improve context handling #4453

Merged
merged 66 commits into from May 25, 2018
Merged

Improve context handling #4453

merged 66 commits into from May 25, 2018

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 24, 2018

This addresses multiple issues with the contexts and document manager, including #4443, #3495

  • Make Notebook inherit from DocumentWidget?
    • Handle the Notebook panel class better. Right now, for example, you can access the notebook both with .content and .notebook. Perhaps deprecate .notebook?
    • Right now you can change the context on an existing notebook. When do we use that? We can't do that for document widgets in general. I changed this to only set a context at construction
    • Remove a lot of layout/focus/other logic we've pushed up to DocumentWidget.
    • Fix TODO in the notebook constructor - some logic about focus and undo state is not being run at the right time. (jasongrout@55f8b32#diff-30a0e8e264c5bcc2eeae28f7c78829b8R125)
    • Fix skipped notebook tests
    • Fix many references to the .notebook attribute of Notebook panels.
    • Notebook panels reset the model undo state - but should not since the same model can be used for different views (
      // Use an existing context if available.
      context = this._findContext(path, factory.name) || null;
      ), and we don't want to reset the undo state for each new view. Instead, undo state should be reset by the context that holds the model.
  • Audit this system to make sure it satisfies the goals of the original proposal regarding loading widgets quickly when jlab first comes up Different PR - see next item.
  • Make sure we take care of the Context section of the latest version of our design (like the context heading of #3495 (comment)) Moving to different PR.
  • Remove comment monologues
  • Solve various TODOs noted in the code
  • Fix the csv viewer toolbar, which is a custom toolbar in the actual content widget. Change to just add a delimiter button to the toolbar. Perhaps this should be done in a different PR after @gnestor's toolbar work is in?
  • Revamp document docs for these changes.

Design discussions:

  • Insist that IDocumentWidget content has a ready promise? Right now we wait until the context is ready, but that doesn't mean the document widget is ready. No, don't insist on the content having a promise - let that be a case-by-case decision for now.
  • Design discussion: eliminate document extension widgets in favor of more explicit InstanceTrackers being provided to the system? See #3974. This work might be better to push to a different PR since this is already getting big and complicated. Move this to a different PR.
  • Design discussion: Once we upgrade to TS 2.8, we'll have a nicer way for document widget subclasses to provide default content. See cbb1b6a for an example of how we can do it now. Upgrade happened on a different PR
  • Design discussion: how much should we push attributes up to the document widget, or should the convention be to access things from the .content attribute? See for example many places where for the file editor actions, we have to do widget.content.editor. Don't push up attributes - let people go to .content to interact with the content widget.
@jasongrout jasongrout force-pushed the context branch 2 times, most recently from 791c149 to e0ef794 Apr 26, 2018
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Apr 26, 2018

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

jasongrout added 2 commits May 9, 2018
…ic into the factory.

This uses basic parts in a composable way, but puts a lot of burden on the factory and externalizes a lot of the logic that maybe should be inside the document widget.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 12, 2018

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 do widget.content.editor to get to the editor, whereas before they just did widget.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.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 12, 2018

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

set context(newValue: DocumentRegistry.IContext<INotebookModel>) {

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 the model into the constructor, versus setting the model after constructing the object, but I don't remember the context being part of that discussion

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 24, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.ts
index 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;

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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

diff --git a/packages/docregistry/src/default.ts b/packages/docregistry/src/default.ts
index 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;

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.
 */
export
function sleep<T>(milliseconds: number = 0, value?: T): Promise<T> {
  return new Promise((resolve, reject) => {
    setTimeout(() => { resolve(value); }, milliseconds);
  });
}

/**
 * Return a promise that resolves in the next animatino frame with the particular value.
 */
export
function moment<T>(value?: T): Promise<T> {
  return new Promise((resolve, reject) => {
    requestAnimationFrame(() => { resolve(value); });
  });
}

Then making options.reveal on line 420ish sleep(0) has a short load time, moment() has a short load time, and sleep(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.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

Hmmm, sounds like another div may be necessary.

jasongrout added 2 commits May 25, 2018
…than a full phosphor widget.

We are seeing significant slowdowns opening large (megabytes) one-line code editor files, where it seems that setting the large text value before putting the codemirror on the page is drastically slowing things down. This approach puts the widget on the page immediately (thus avoiding the slowdown), and just covers it with a spinner until it can be revealed.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

What prompted you to change your mind on whether content could be null?

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks great!

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented May 25, 2018

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.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

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

@ian-r-rose ian-r-rose merged commit 363b966 into jupyterlab:master May 25, 2018
1 of 2 checks passed
@jasongrout jasongrout deleted the context branch May 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants