Fix code lens perf for interactive window #11142

rchiodo · 2020-04-13T22:58:44Z

For #10971

Fix the perf of code lens generation by changing how we do a bunch of things.

Don't regen the code lens twice on every execute. Only necessary to do it once (when the cell is finished). Used to also do it when the hash was updated (during pre execute)
Cache code lens ranges based on document version number. Not necessary to recompute if a file hasn't actually changed. Computing cell ranges is expensive as it does a regex on every line of the file.
Cache code lens for the ranges based on the same version number. Not necessary to recreate unless the user changes the file or changes their settings.
Cache the 'goto' code lens based on if a document has executed any code or not.

Also got rid of the cellhashlogger as it was unnecessary. The cellHashProvider is a notebook execution logger. It doesn't need the in between piece.

rchiodo · 2020-04-13T23:16:58Z

src/client/datascience/editor-integration/codewatcher.ts

@@ -14,6 +14,7 @@ import {
    TextEditorRevealType
 } from 'vscode';

+import { IDisposable } from 'monaco-editor';


IDisposable [](start = 9, length = 11)

Crap, Wrong one.

rchiodo · 2020-04-13T23:19:26Z

src/client/datascience/interactive-window/interactiveWindow.ts

-        return Uri.parse(Identifiers.InteractiveWindowIdentity);
+        return {
+            resource: Uri.parse(Identifiers.InteractiveWindowIdentity),
+            type: 'interactive'


We need a type here so that the codeLensFactory can only initiailize it's hash provider when the notebook for the interactive window is created. It's a singleton so we don't want to reinit the hash provider on every notebook open.

DonJayamanne · 2020-04-13T23:16:53Z

src/client/datascience/editor-integration/cellhashprovider.ts

@@ -101,6 +81,12 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi
            .filter((e) => e.hashes.length > 0);
    }

+    public restartedKernel() {


Suggested change

public restartedKernel() {

public onKernelRestarted() {

As we're reacting to an event that took place (i.e. this is meant to be an event handler), I think onKernelRestarted is better

Sure.

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

codecov-io · 2020-04-13T23:35:36Z

Codecov Report

Merging #11142 into master will decrease coverage by 61.47%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #11142       +/-   ##
===========================================
- Coverage   61.47%    0.00%   -61.48%     
===========================================
  Files         597       11      -586     
  Lines       32801       37    -32764     
  Branches     4640        4     -4636     
===========================================
- Hits        20163        0    -20163     
+ Misses      11622       37    -11585     
+ Partials     1016        0     -1016

Impacted Files	Coverage Δ
src/client/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/debugger/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/common/process/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/common/platform/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/interpreter/autoSelection/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/interpreter/locators/services/conda.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...t/datascience/jupyter/jupyterDataRateLimitError.ts	`0.00% <0.00%> (-50.00%)`	⬇️
src/client/datascience/commands/serverSelector.ts
...bugger/extension/adapter/outdatedDebuggerPrompt.ts
src/client/providers/objectDefinitionProvider.ts
... and 569 more

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 56051b8...18f7582. Read the comment docs.

IanMatthewHuff · 2020-04-13T23:41:27Z

src/client/datascience/editor-integration/codeLensFactory.ts

-        }
+    private onChangedSettings() {
+        // When config settings change, refresh our code lenses.
+        this.codeLensCache.clear();


This looks like it's on any settings change.

I think there is that .affectConfiguration function we should be using here. Or else we are clearing these out too much.

No there isn't. Not on the config settings. That's on the workspaceConfiguration.

And the worst case is that the code lens are regenerated whenever you change your python settings. Meh.

You can see where the configSettings uses that 'affectsConfiguration' in the configSettings.ts file (line 571 or so). It will only fire an onDidChangeSettings when the python settings change.

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

We should probably recompute the code lenses though. Otherwise the next execution will recompute them all.

In reply to: 407782955 [](ancestors = 407782955,407782405)

IanMatthewHuff · 2020-04-13T23:45:26Z

src/client/datascience/editor-integration/codewatcher.ts

+        this.updateRequiredDisposable = this.codeLensFactory.updateRequired(this.onCodeLensFactoryUpdated.bind(this));
+
+        // Make sure to stop listening for changes when this document closes.
+        this.documentManager.onDidCloseTextDocument(this.onDocumentClosed.bind(this));


This is disposable.

Whoops, will fix.

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

sonarqubecloud · 2020-04-13T23:52:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

* Eliminate first level of redundancy * Working with a cache * Add test to verify no more generating cell ranges * Add news entry * Clean up on closing * Make sure close and reopen works * Fix sonar errors * Fix restart * Fix wrong IDisposable type * Rename restartedKernel to onKernelRestarted * Recompute when changing settings * Clear out document close event when document closes

* Fix code lens perf for interactive window (#11142) * Eliminate first level of redundancy * Working with a cache * Add test to verify no more generating cell ranges * Add news entry * Clean up on closing * Make sure close and reopen works * Fix sonar errors * Fix restart * Fix wrong IDisposable type * Rename restartedKernel to onKernelRestarted * Recompute when changing settings * Clear out document close event when document closes * Update changelog * Fix build error from missing file

rchiodo added 6 commits April 13, 2020 12:57

Eliminate first level of redundancy

18d8968

Working with a cache

f02d847

Add test to verify no more generating cell ranges

6be574a

Add news entry

82605b0

Clean up on closing

09c2b49

Make sure close and reopen works

d17058b

rchiodo requested review from DavidKutu, DonJayamanne, greazer and IanMatthewHuff April 13, 2020 22:58

rchiodo self-assigned this Apr 13, 2020

rchiodo added 2 commits April 13, 2020 16:04

Fix sonar errors

1806922

Fix restart

7d5a6dc

rchiodo commented Apr 13, 2020

View reviewed changes

Fix wrong IDisposable type

916d77b

rchiodo commented Apr 13, 2020

View reviewed changes

DonJayamanne approved these changes Apr 13, 2020

View reviewed changes

Rename restartedKernel to onKernelRestarted

f1e7138

IanMatthewHuff reviewed Apr 13, 2020

View reviewed changes

rchiodo added 2 commits April 13, 2020 16:48

Recompute when changing settings

6c87fb1

Clear out document close event when document closes

18f7582

IanMatthewHuff approved these changes Apr 13, 2020

View reviewed changes

rchiodo merged commit eab579f into master Apr 14, 2020

rchiodo deleted the rchiodo/codelens_perf branch April 14, 2020 00:23

rchiodo mentioned this pull request Apr 14, 2020

Port perf fix to release branch #11144

Merged

lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix code lens perf for interactive window #11142

Fix code lens perf for interactive window #11142

Uh oh!

rchiodo commented Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

DonJayamanne Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

codecov-io commented Apr 13, 2020 •

edited

Loading

Uh oh!

IanMatthewHuff Apr 13, 2020

Uh oh!

IanMatthewHuff Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

IanMatthewHuff Apr 13, 2020

Uh oh!

rchiodo Apr 13, 2020

Uh oh!

sonarqubecloud bot commented Apr 13, 2020

Uh oh!

Uh oh!

Fix code lens perf for interactive window #11142

Fix code lens perf for interactive window #11142

Uh oh!

Conversation

rchiodo commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 13, 2020

Uh oh!

Uh oh!

codecov-io commented Apr 13, 2020 •

edited

Loading