Speed up Out Of Scope lines by jasonLaster · Pull Request #3184 · firefox-devtools/debugger

jasonLaster · 2017-06-18T03:46:46Z

Associated Issue: #3180

Summary of Changes

This refactors out of scope lines a bit to support getting a list of in scope lines that can be used by the editor to check if a token is in scope on hover.

Test Plan

Jest tests

codecov · 2017-06-18T04:54:33Z

Codecov Report

Merging #3184 into master will decrease coverage by 0.03%.
The diff coverage is 67.5%.

@@            Coverage Diff             @@
##           master    #3184      +/-   ##
==========================================
- Coverage   47.59%   47.55%   -0.04%     
==========================================
  Files          97       98       +1     
  Lines        4036     4052      +16     
  Branches      828      832       +4     
==========================================
+ Hits         1921     1927       +6     
- Misses       2115     2125      +10

Impacted Files	Coverage Δ
src/utils/scopes.js	`80.35% <0%> (-2.98%)`	⬇️
src/reducers/ast.js	`83.78% <0%> (-4.79%)`	⬇️
src/components/Editor/index.js	`18.35% <0%> (-1.81%)`	⬇️
src/actions/sources.js	`50.62% <100%> (ø)`	⬆️
src/actions/ast.js	`72.09% <91.66%> (-0.41%)`	⬇️
src/selectors/linesInScope.js	`93.75% <93.75%> (ø)`
src/reducers/sources.js	`76.35% <0%> (-2.03%)`	⬇️

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 38e1558...7a85c3c. Read the comment docs.

codecov · 2017-06-18T04:54:33Z

Codecov Report

Merging #3184 into master will increase coverage by 0.03%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #3184      +/-   ##
==========================================
+ Coverage   47.59%   47.63%   +0.04%     
==========================================
  Files          97       98       +1     
  Lines        4036     4062      +26     
  Branches      828      837       +9     
==========================================
+ Hits         1921     1935      +14     
- Misses       2115     2127      +12

Impacted Files	Coverage Δ
src/utils/scopes.js	`83.33% <ø> (ø)`	⬆️
src/reducers/ast.js	`83.78% <0%> (-4.79%)`	⬇️
src/components/Editor/index.js	`19.31% <27.27%> (-0.85%)`	⬇️
src/actions/ast.js	`71.42% <75%> (-1.08%)`	⬇️
src/selectors/linesInScope.js	`93.75% <93.75%> (ø)`

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 22b7436...47fac1d. Read the comment docs.

jasonLaster · 2017-06-18T03:49:31Z

+
+    if (!linesInScope.includes(line)) {
+      return;
+    }


this was the main thing that I wanted to get out of this PR. previously we were checking the parent's class which required always doing an expensive render. We should be ideally using data which is more trustworthy.

jasonLaster · 2017-06-18T04:55:24Z

-    if (this.props.outOfScopeLocations !== nextProps.outOfScopeLocations) {
-      clearLineClass(this.editor.codeMirror, "out-of-scope");
+    if (this.props.linesInScope !== nextProps.linesInScope) {
+      this.editor.codeMirror.operation(() => {


this.editor.codeMirror.operation is a huge perf win. With that said, i'll be moving the operation inside clearLineClass soon :)

jasonLaster · 2017-06-18T04:56:59Z

+  renderInScopeLines() {
+    const { linesInScope } = this.props;
+    if (
+      !isEnabled("highlightScopeLines") ||


i'm re-adding a feature flag because i'm not happy with the cases we're missing like blocks and comments. Seeing those issues makes me think we should wait and re-launch highlighting when it's further along.

With that said, we still get the onMouseOver logic so fewer accidental popups

jasonLaster · 2017-06-18T04:58:26Z

@@ -0,0 +1,42 @@
+// import { linesInScope } from "../utils/scopes";
+import { getOutOfScopeLocations, getSelectedSource } from "../selectors";


this is the first selector that spans stores (sources, ast) with that in mind I created a new selectors directory. I'm not sure if that makes sense. What do others think?

jasonLaster · 2017-06-18T04:59:31Z

-    const sourceText = getSourceText(getState(), source.id);
-    if (!sourceText) {
+    const source = sourceRecord.toJS();
+    if (!source.text || hasSymbols(getState(), source)) {


some small sourceText => source migrations

jasonLaster · 2017-06-19T11:42:52Z

-      "column": 21,
-      "line": 1,
+      "column": 3,
+      "line": 10,


I switched the test to read from a new fixture file :)

jasonLaster · 2017-06-19T11:43:03Z

+  10,
+  11,
+  12,
+]


I added the ability to do line reviews

jasonLaster · 2017-06-19T12:08:25Z

-      await dispatch(actions.loadSourceText({ id: "base.js" }));
-      await dispatch(actions.selectSource("base.js", { line: 1 }));
-
-      await dispatch(actions.setOutOfScopeLocations());


yeah - so this is coooool. Turns out many of the dispatches are unnecessary.

jasonLaster · 2017-06-19T12:26:28Z

+}
+
+.paused .CodeMirror-line, .paused .CodeMirror-linenumber {
+  opacity: 0.7;


we're now tracking in-scope and not out-of-scope.

some reasons:

it's easier to get the lines in scope than all the lines that are out of scope

it should be faster because there are fewer innies than outies.

jasonLaster · 2017-06-19T12:27:12Z

+    case "RESUMED": {
+      return state.set("outOfScopeLocations", null);
+    }
+


this fixes #3123

codehag

Looks pretty good. I have a few comments. let me know if you have any questions!

codehag · 2017-06-19T12:44:54Z

@@ -0,0 +1,41 @@
+import { getOutOfScopeLocations, getSelectedSource } from "../selectors";
+
+import range from "lodash/range";


would it make sense to do this?

import { range, flatMap, uniq, without } from "lodash";

sadly... no because then we pull lodash into the bundle... I suppose if we made it an external then we could exclude it from the bundle and expect it to be available in m-c... If we like this i can file a separate issue for it.

codehag · 2017-06-19T12:46:16Z

+import without from "lodash/without";
+
+function getLinesOutOfScope(outOfScopeLocations: AstLocation[]) {
+  if (!outOfScopeLocations) {


if this is an array of AstLocations, I think this will always be true... or maybe it isnt an array?

if it is an array we probably need to do

if (!outOfScopeLocations.length) { ... }

I can do both. when it is not available it's nully.

codehag · 2017-06-19T12:47:10Z

+import uniq from "lodash/uniq";
+import without from "lodash/without";
+
+function getLinesOutOfScope(outOfScopeLocations: AstLocation[]) {


getLinesOutOfScope sounds like we are taking lines out of a scope, but I think this is getting the out of scope lines. maybe getOutOfScopeLines will be clearer here,

codehag · 2017-06-19T12:47:38Z

+
+  return uniq(
+    flatMap(outOfScopeLocations, location =>
+      range(location.start.line, location.end.line)


codehag · 2017-06-19T12:48:12Z

+  );
+}
+
+export default function getLinesInScope(state: OuterState) {


same comment here about naming: proposing getInScopeLines

codehag · 2017-06-19T12:49:19Z

+
+function getLinesOutOfScope(outOfScopeLocations: AstLocation[]) {
+  if (!outOfScopeLocations) {
+    return null;


this is returning a different type; maybe it should be an empty array?

... not sure. i kinda like having explicit empty states to distinguish between nothing and we happen to have no out of scope locs. ill think about it

codehag · 2017-06-19T12:49:41Z

+  const sourceNumLines = source.get("text").split("\n").length;
+  const sourceLines = range(1, sourceNumLines + 1);
+
+  if (!linesOutOfScope) {


the logic here would be clearer if this was checking for an empty array

codehag · 2017-06-19T12:50:05Z

-      await dispatch(actions.loadSourceText({ id: "base.js" }));
-      await dispatch(actions.selectSource("base.js", { line: 1 }));
-
-      await dispatch(actions.setOutOfScopeLocations());


codehag

LGTM!

jasonLaster commented Jun 18, 2017

View reviewed changes

jasonLaster force-pushed the oos branch 2 times, most recently from 3423f5e to 539c2ae Compare June 19, 2017 11:53

jasonLaster commented Jun 19, 2017

View reviewed changes

codehag reviewed Jun 19, 2017

View reviewed changes

jasonLaster force-pushed the oos branch from 539c2ae to 20ef451 Compare June 19, 2017 13:11

Add the ability to derive the lines in scope

300905b

jasonLaster force-pushed the oos branch from 20ef451 to 300905b Compare June 19, 2017 13:23

codehag approved these changes Jun 19, 2017

View reviewed changes

small refactor

47fac1d

jasonLaster merged commit 80dd3c8 into firefox-devtools:master Jun 19, 2017

		@@ -0,0 +1,42 @@
		// import { linesInScope } from "../utils/scopes";
		import { getOutOfScopeLocations, getSelectedSource } from "../selectors";

+,
+,
+,
+              ]

		@@ -0,0 +1,41 @@
		import { getOutOfScopeLocations, getSelectedSource } from "../selectors";

		import range from "lodash/range";

Conversation

jasonLaster commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Test Plan

Uh oh!

codecov Bot commented Jun 18, 2017

Codecov Report

Uh oh!

codecov Bot commented Jun 18, 2017 • 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!

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!

codehag left a comment

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!

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!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jasonLaster commented Jun 18, 2017 •

edited

Loading

codecov Bot commented Jun 18, 2017 •

edited

Loading