Fix a number of perf issues with large notebooks #7778

rchiodo · 2019-10-04T21:54:23Z

For #7483

Made a number of adjustments to increase perf

Change load all cells to not send updates on each cell added. Wait till they're all done
Change render of all cells to not use global state, this allows us to skip rendering cells that haven't changed
Rework intellisense to not react to sendInfo messages, but instead react to specific changes. SendInfo happens too often and is too expensive to parse constantly
Rework monaco editor to not update widget parents until absolutely necessary

Added some more unit tests for the intellisense changes.

There's still a bunch of work left to do though. We need to virtualize the monaco editors somehow as they are the big bottleneck on opening now. A notebook with 500 cells in it takes 10 seconds just to show the first render. Resizing after that requires 5 seconds after resizing the window to calm down.

Change updates to not update full cell Change global properties to be per cell Change cells to check for updates before rendering

Some perf fixes for monaco editor. Skip laying out parent etc until necessary

Fix markdown editor background

rchiodo · 2019-10-04T22:01:19Z

src/datascience-ui/native-editor/nativeCell.tsx

+        // Indicate not showing the editor anymore. The equivalent of this
+        // is not when we receive focus but when we GIVE focus to the markdown editor
+        // otherwise we wouldn't be able to display it.
+        this.setState({showingMarkdownEditor: false});


This should be unnecessary as I changed the logic here. It uses the props to determine if showing the markdown editor or not. I'm going to remove this and make sure markdown editing still works #Resolved

rchiodo · 2019-10-04T22:03:55Z

src/datascience-ui/native-editor/nativeEditor.tsx

                        } else {
-                            cellVMs[i] = cloneDeep(cellVM);
-                            cellVMs[i].useQuickEdit = false;
+                            cellVMs[i] = immutable.merge(cellVM, { useQuickEdit: false });


merge [](start = 51, length = 5)

crap, this is wrong. Merge is unnecessary here. #Resolved

rchiodo · 2019-10-04T22:06:31Z

                this.updateWidgetParent(this.state.editor);

Hmm, might be able to eliminate this. It would make resizing all of the controls faster. #Resolved

Refers to: src/datascience-ui/react-common/monacoEditor.tsx:389 in c590780. [](commit_id = c590780, deletion_comment = False)

codecov-io · 2019-10-04T23:01:17Z

Codecov Report

Merging #7778 into master will increase coverage by 0.42%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master    #7778      +/-   ##
==========================================
+ Coverage   58.75%   59.17%   +0.42%     
==========================================
  Files         496      498       +2     
  Lines       22158    22245      +87     
  Branches     3570     3573       +3     
==========================================
+ Hits        13019    13164     +145     
+ Misses       8321     8259      -62     
- Partials      818      822       +4

Impacted Files	Coverage Δ
...ience/interactive-common/interactiveWindowTypes.ts	`100% <100%> (ø)`	⬆️
src/datascience-ui/interactive-common/mainState.ts	`44.89% <66.66%> (ø)`
...ve-common/intellisense/baseIntellisenseProvider.ts	`37.35% <85.71%> (+10.41%)`	⬆️
...active-common/intellisense/intellisenseDocument.ts	`73.33% <95.91%> (+35.77%)`	⬆️
...client/activation/languageServer/languageServer.ts	`90.14% <0%> (-1.93%)`	⬇️
src/client/activation/languageServer/constants.ts	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a155680...f3b44ba. Read the comment docs.

IanMatthewHuff · 2019-10-07T16:48:33Z

src/datascience-ui/interactive-common/mainStateController.ts

-        const cellVM = this.state.cellVMs.find(c => c.cell.id === cellId);
-        if (cellVM) {
+        const index = this.findCellIndex(cellId);
+        if (index >= 0) {


Is it right here to be recomputing selection and focus if the deleted cell is either selected or focused? Will focused context ever not be in the selected cell? #WontFix

You mean the or statement? (this.pendingState.selectedCell === cellId || this.pendingState.focusedCell === cellId)?

The second half of this should always be true if the first half is, but it's just in case the logic gets messed up somewhere else.

In reply to: 332125334 [](ancestors = 332125334)

Yeah, I wasn't sure if that was a tautology or not.

Like via this:
if (newSelect >= 0) {
newVMs[newSelect] = { ...newVMs[newSelect], selected: true, focused: focusedCell === newVMs[newSelect].cell.id };
}

In reply to: 332129126 [](ancestors = 332129126,332125334)

Oh we only want to change it if the selected item is the item being deleted, so it would be more like this:

if (newSelection === cellId)

In reply to: 332130707 [](ancestors = 332130707,332129126,332125334)

IanMatthewHuff · 2019-10-07T17:10:51Z

src/datascience-ui/native-editor/nativeCell.tsx

+        const content = this.isMarkdownCell() && !this.isShowingMarkdownEditor() ?
            <div className='cell-result-container'>
                <div className='cell-row-container'>
                    {this.renderCollapseBar(false)}


This lets you delete a cell without selecting it? #WontFix

Yeah it should. You hover over it and click delete

In reply to: 332135165 [](ancestors = 332135165)

DonJayamanne

Still a little confused with what's going on.
Thats primarily due to lack of domain knowledge.
Added a few comments, approving as they aren't show stoppers.

Adding a few comments in some places should address most of my concerns.

src/client/datascience/interactive-common/intellisense/baseIntellisenseProvider.ts

DonJayamanne · 2019-10-07T16:54:50Z

src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts

+            // Return our changes
+            changes = [
+                {
+                    range: this.createSerializableRange(new Position(0, 0), new Position(0, 0)),


Wouldn't it be better to create a custom serializer/transformer. If tomorrow we forget to use this, then we wouldn't know exactly what wen't wrong. Or if another class has the same problem as Position.

Sorry don't have the foggiest what you're referring to. You mean the function createSerializableRange?

In reply to: 332128183 [](ancestors = 332128183)

Yes. I'm assuming we are using createSerializableRange, because JSON.stringify faills or puts a lot of unwanted guff.

src/datascience-ui/react-common/monacoEditor.tsx

DonJayamanne · 2019-10-07T17:00:41Z

src/datascience-ui/native-editor/nativeEditor.tsx

-                        hideOutput={cellVM.hideOutput}
                        focusCell={this.focusCell}
                        selectCell={this.selectCell}
+                        lastCell={lastLine !== null}


Wouldn't it be more efficient to track the selected cellId instead of the object!
Same applies to focusCell, selectedCell, lastCell #WontFix

Will improve overall perf in react too, as they are simple strings that are being passed around. #WontFix

Those values are the cell id. They're not the objects.

In reply to: 332130854 [](ancestors = 332130854)

src/datascience-ui/native-editor/nativeCell.tsx

src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts

src/client/datascience/interactive-common/interactiveWindowTypes.ts

IanMatthewHuff

* First idea - load cells all at once. Change updates to not update full cell Change global properties to be per cell Change cells to check for updates before rendering * Fix delete to not select at the same time * Make a pending state and a rendered state to use for rendering Some perf fixes for monaco editor. Skip laying out parent etc until necessary * Refactor intellisense to not take so much time on load * More delete and move fixes for intellisense * Fix markdown not switching Fix markdown editor background * Fix some of the time on first render * Add some more unit tests and fix intellisense issue * Add news entry and fix variable explorer open * Remove unnecessary state change * Remove unnecessary merge * Comment change * Postpone widget parent changes * Add new dependency * Update some comments and names to be clearer

rchiodo added 10 commits October 3, 2019 13:26

First idea - load cells all at once.

be69c1c

Change updates to not update full cell Change global properties to be per cell Change cells to check for updates before rendering

Fix delete to not select at the same time

34e1d41

Make a pending state and a rendered state to use for rendering

9534968

Some perf fixes for monaco editor. Skip laying out parent etc until necessary

Refactor intellisense to not take so much time on load

92f62b0

More delete and move fixes for intellisense

d5f7f21

Fix markdown not switching

6548160

Fix markdown editor background

Fix some of the time on first render

af910f6

Merge remote-tracking branch 'origin/master' into rchiodo/perf_step_1

e5bd827

Add some more unit tests and fix intellisense issue

28dc348

Add news entry and fix variable explorer open

c590780

rchiodo requested review from DavidKutu, DonJayamanne and IanMatthewHuff October 4, 2019 21:54

rchiodo self-assigned this Oct 4, 2019

rchiodo commented Oct 4, 2019

View reviewed changes

Remove unnecessary state change

294483a

rchiodo commented Oct 4, 2019

View reviewed changes

rchiodo added 2 commits October 4, 2019 15:04

Remove unnecessary merge

39fdcaf

Comment change

d5dc127

rchiodo added 2 commits October 4, 2019 15:22

Postpone widget parent changes

3b7baf9

Add new dependency

1911469

Merge from master

5b58206

IanMatthewHuff reviewed Oct 7, 2019

View reviewed changes

DonJayamanne approved these changes Oct 7, 2019

View reviewed changes

IanMatthewHuff approved these changes Oct 7, 2019

View reviewed changes

Update some comments and names to be clearer

f3b44ba

rchiodo merged commit 218047c into master Oct 7, 2019

rchiodo deleted the rchiodo/perf_step_1 branch October 7, 2019 18:45

lock bot locked as resolved and limited conversation to collaborators Oct 14, 2019

Fix a number of perf issues with large notebooks #7778

Fix a number of perf issues with large notebooks #7778

Uh oh!

Conversation

rchiodo commented Oct 4, 2019

Uh oh!

rchiodo Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IanMatthewHuff Oct 7, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Oct 7, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DonJayamanne Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DonJayamanne Oct 7, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rchiodo Oct 4, 2019 •

edited

Loading

rchiodo Oct 4, 2019 •

edited

Loading

rchiodo commented Oct 4, 2019 •

edited

Loading

codecov-io commented Oct 4, 2019 •

edited

Loading

IanMatthewHuff Oct 7, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Oct 7, 2019 •

edited

Loading

IanMatthewHuff Oct 7, 2019 •

edited by rchiodo

Loading

DonJayamanne Oct 7, 2019 •

edited by rchiodo

Loading

DonJayamanne Oct 7, 2019 •

edited by rchiodo

Loading