Bug 1531350 - [release 128] [Breakpoints] memoize breakpoint position fetching (#8001). r=dwalsh
authorJason Laster <jason.laster.11@gmail.com>
Thu, 28 Feb 2019 14:37:27 -0500
changeset 520007 9560b0da49d4316b8f543f51d9eb133d0b38ace8
parent 520006 2856ab932b13e044656067702612b36c1f40d13a
child 520008 b6811a8e2c6eec0336ab462e8f46d3c800041416
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdwalsh
bugs1531350
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1531350 - [release 128] [Breakpoints] memoize breakpoint position fetching (#8001). r=dwalsh
devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
devtools/client/debugger/new/src/actions/breakpoints/breakpointPositions.js
devtools/client/debugger/new/src/actions/breakpoints/index.js
devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
devtools/client/debugger/new/src/actions/breakpoints/tests/breakpointPositions.spec.js
devtools/client/debugger/new/src/actions/types/BreakpointAction.js
devtools/client/debugger/new/src/reducers/breakpoints.js
devtools/client/debugger/new/src/utils/defer.js
--- a/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js
@@ -125,17 +125,17 @@ export function enableBreakpoint(breakpo
 export function addBreakpoint(
   location: SourceLocation,
   options: BreakpointOptions = {}
 ) {
   return async ({ dispatch, getState, sourceMaps, client }: ThunkArgs) => {
     recordEvent("add_breakpoint");
     let breakpointPosition = location;
     if (features.columnBreakpoints && location.column === undefined) {
-      await dispatch(setBreakpointPositions(location));
+      await dispatch(setBreakpointPositions(location.sourceId));
       breakpointPosition = getFirstVisibleBreakpointPosition(
         getState(),
         location
       );
     }
 
     if (!breakpointPosition) {
       return;
--- a/devtools/client/debugger/new/src/actions/breakpoints/breakpointPositions.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/breakpointPositions.js
@@ -1,25 +1,23 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
 
 import { isOriginalId, originalToGeneratedId } from "devtools-source-map";
 
-import {
-  getSourceFromId,
-  getBreakpointPositionsForSource
-} from "../../selectors";
+import { getSourceFromId, hasBreakpointPositions } from "../../selectors";
 
-import type { SourceLocation } from "../../types";
 import type { ThunkArgs } from "../../actions/types";
 import { getOriginalLocation } from "../../utils/source-maps";
 
+const requests = new Map();
+
 async function mapLocations(generatedLocations, state, source, sourceMaps) {
   return Promise.all(
     generatedLocations.map(async generatedLocation => {
       const location = await getOriginalLocation(
         generatedLocation,
         source,
         sourceMaps
       );
@@ -36,56 +34,77 @@ function convertToList(results, sourceId
     for (const column of results[line]) {
       positions.push({ line: Number(line), column: column, sourceId });
     }
   }
 
   return positions;
 }
 
-export function setBreakpointPositions(location: SourceLocation) {
-  return async ({ getState, dispatch, client, sourceMaps }: ThunkArgs) => {
-    let sourceId = location.sourceId;
-    if (getBreakpointPositionsForSource(getState(), sourceId)) {
+async function getBreakpointPositions(
+  sourceId,
+  { client, dispatch, getState, sourceMaps }
+) {
+  let source = getSourceFromId(getState(), sourceId);
+
+  let results = {};
+  if (isOriginalId(sourceId)) {
+    const ranges = await sourceMaps.getGeneratedRangesForOriginal(
+      sourceId,
+      source.url,
+      true
+    );
+    sourceId = originalToGeneratedId(sourceId);
+    source = getSourceFromId(getState(), sourceId);
+
+    // Note: While looping here may not look ideal, in the vast majority of
+    // cases, the number of ranges here should be very small, and is quite
+    // likely to only be a single range.
+    for (const range of ranges) {
+      // Wrap infinite end positions to the next line to keep things simple
+      // and because we know we don't care about the end-line whitespace
+      // in this case.
+      if (range.end.column === Infinity) {
+        range.end.line += 1;
+        range.end.column = 0;
+      }
+
+      const bps = await client.getBreakpointPositions(source.actors[0], range);
+      for (const line in bps) {
+        results[line] = (results[line] || []).concat(bps[line]);
+      }
+    }
+  } else {
+    results = await client.getBreakpointPositions(source.actors[0]);
+  }
+
+  const positions = convertToList(results, sourceId);
+  return mapLocations(positions, getState(), source, sourceMaps);
+}
+
+export function setBreakpointPositions(sourceId: string) {
+  return async (thunkArgs: ThunkArgs) => {
+    const { dispatch, getState } = thunkArgs;
+
+    if (hasBreakpointPositions(getState(), sourceId)) {
       return;
     }
 
-    let source = getSourceFromId(getState(), sourceId);
-
-    let results = {};
-    if (isOriginalId(sourceId)) {
-      const ranges = await sourceMaps.getGeneratedRangesForOriginal(
+    if (!requests.has(sourceId)) {
+      requests.set(
         sourceId,
-        source.url,
-        true
+        (async () => {
+          try {
+            dispatch({
+              type: "ADD_BREAKPOINT_POSITIONS",
+              sourceId,
+              positions: await getBreakpointPositions(sourceId, thunkArgs)
+            });
+          } finally {
+            requests.delete(sourceId);
+          }
+        })()
       );
-      sourceId = originalToGeneratedId(sourceId);
-      source = getSourceFromId(getState(), sourceId);
-
-      // Note: While looping here may not look ideal, in the vast majority of
-      // cases, the number of ranges here should be very small, and is quite
-      // likely to only be a single range.
-      for (const range of ranges) {
-        // Wrap infinite end positions to the next line to keep things simple
-        // and because we know we don't care about the end-line whitespace
-        // in this case.
-        if (range.end.column === Infinity) {
-          range.end.line += 1;
-          range.end.column = 0;
-        }
-
-        const bps = await client.getBreakpointPositions(
-          source.actors[0],
-          range
-        );
-        for (const line in bps) {
-          results[line] = (results[line] || []).concat(bps[line]);
-        }
-      }
-    } else {
-      results = await client.getBreakpointPositions(source.actors[0]);
     }
 
-    let positions = convertToList(results, sourceId);
-    positions = await mapLocations(positions, getState(), source, sourceMaps);
-    return dispatch({ type: "ADD_BREAKPOINT_POSITIONS", positions, location });
+    await requests.get(sourceId);
   };
 }
--- a/devtools/client/debugger/new/src/actions/breakpoints/index.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/index.js
@@ -42,16 +42,18 @@ import type {
   BreakpointOptions,
   Source,
   SourceLocation,
   XHRBreakpoint
 } from "../../types";
 
 import { recordEvent } from "../../utils/telemetry";
 
+export * from "./breakpointPositions";
+
 async function removeBreakpointsPromise(client, state, breakpoint) {
   const breakpointLocation = makeBreakpointLocation(
     state,
     breakpoint.generatedLocation
   );
   await client.removeBreakpoint(breakpointLocation);
 }
 
--- a/devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
+++ b/devtools/client/debugger/new/src/actions/breakpoints/syncBreakpoint.js
@@ -12,17 +12,17 @@ import {
   assertPendingBreakpoint,
   findScopeByName,
   makeBreakpointLocation
 } from "../../utils/breakpoint";
 
 import { getGeneratedLocation } from "../../utils/source-maps";
 import { getTextAtPosition } from "../../utils/source";
 import { originalToGeneratedId, isOriginalId } from "devtools-source-map";
-import { getSource } from "../../selectors";
+import { getSource, getBreakpointPositionsForSource } from "../../selectors";
 import { features } from "../../utils/prefs";
 
 import type { ThunkArgs, Action } from "../types";
 
 import type {
   SourceLocation,
   ASTLocation,
   PendingBreakpoint,
@@ -30,25 +30,27 @@ import type {
   Breakpoint
 } from "../../types";
 
 type BreakpointSyncData = {
   previousLocation: SourceLocation,
   breakpoint: ?Breakpoint
 };
 
-async function isPossiblePosition(location, dispatch) {
-  if (features.columnBreakpoints && location.column != undefined) {
-    const { positions } = await dispatch(setBreakpointPositions(location));
-    if (!positions.some(({ generatedLocation }) => generatedLocation.column)) {
-      return false;
-    }
+async function isPossiblePosition(state, location, dispatch) {
+  if (!features.columnBreakpoints || location.column != undefined) {
+    return true;
   }
 
-  return true;
+  await dispatch(setBreakpointPositions(location.sourceId));
+  const positions = getBreakpointPositionsForSource(state, location.sourceId);
+  return (
+    positions &&
+    positions.some(({ generatedLocation }) => generatedLocation.column)
+  );
 }
 
 async function makeScopedLocation(
   { name, offset, index }: ASTLocation,
   location: SourceLocation,
   source
 ) {
   const scope = await findScopeByName(source, name, index);
@@ -143,16 +145,17 @@ export async function syncBreakpointProm
   }
 
   const breakpointLocation = makeBreakpointLocation(
     getState(),
     generatedLocation
   );
 
   const possiblePosition = await isPossiblePosition(
+    getState(),
     generatedLocation,
     dispatch
   );
 
   /** ******* CASE 1: No server change ***********/
   // early return if breakpoint is disabled or we are in the sameLocation
   if (possiblePosition && (pendingBreakpoint.disabled || isSameLocation)) {
     // Make sure the breakpoint is installed on all source actors.
new file mode 100644
--- /dev/null
+++ b/devtools/client/debugger/new/src/actions/breakpoints/tests/breakpointPositions.spec.js
@@ -0,0 +1,73 @@
+// @flow
+
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
+
+import {
+  actions,
+  selectors,
+  createStore,
+  makeSource,
+  waitForState
+} from "../../../utils/test-head";
+
+describe("breakpointPositions", () => {
+  it("fetches positions", async () => {
+    const store = createStore({
+      getBreakpointPositions: async () => ({ "9": [1] })
+    });
+
+    const { dispatch, getState } = store;
+    await dispatch(actions.newSource(makeSource("foo")));
+
+    dispatch(actions.setBreakpointPositions("foo"));
+
+    await waitForState(store, state =>
+      selectors.hasBreakpointPositions(state, "foo")
+    );
+
+    expect(
+      selectors.getBreakpointPositionsForSource(getState(), "foo")
+    ).toEqual([
+      {
+        location: { line: 9, column: 1, sourceId: "foo" },
+        generatedLocation: { line: 9, column: 1, sourceId: "foo" }
+      }
+    ]);
+  });
+
+  it("doesn't re-fetch positions", async () => {
+    let resolve = _ => {};
+    let count = 0;
+    const store = createStore({
+      getBreakpointPositions: () =>
+        new Promise(r => {
+          count++;
+          resolve = r;
+        })
+    });
+
+    const { dispatch, getState } = store;
+    await dispatch(actions.newSource(makeSource("foo")));
+
+    dispatch(actions.setBreakpointPositions("foo"));
+    dispatch(actions.setBreakpointPositions("foo"));
+
+    resolve({ "9": [1] });
+    await waitForState(store, state =>
+      selectors.hasBreakpointPositions(state, "foo")
+    );
+
+    expect(
+      selectors.getBreakpointPositionsForSource(getState(), "foo")
+    ).toEqual([
+      {
+        location: { line: 9, column: 1, sourceId: "foo" },
+        generatedLocation: { line: 9, column: 1, sourceId: "foo" }
+      }
+    ]);
+
+    expect(count).toEqual(1);
+  });
+});
--- a/devtools/client/debugger/new/src/actions/types/BreakpointAction.js
+++ b/devtools/client/debugger/new/src/actions/types/BreakpointAction.js
@@ -90,10 +90,10 @@ export type BreakpointAction =
     |}
   | {|
       +type: "REMAP_BREAKPOINTS",
       +breakpoints: Breakpoint[]
     |}
   | {|
       type: "ADD_BREAKPOINT_POSITIONS",
       positions: BreakpointPositions,
-      location: SourceLocation
+      sourceId: string
     |};
--- a/devtools/client/debugger/new/src/reducers/breakpoints.js
+++ b/devtools/client/debugger/new/src/reducers/breakpoints.js
@@ -106,20 +106,17 @@ function update(
       return updateXHRBreakpoint(state, action);
     }
 
     case "DISABLE_XHR_BREAKPOINT": {
       return updateXHRBreakpoint(state, action);
     }
 
     case "ADD_BREAKPOINT_POSITIONS": {
-      const {
-        location: { sourceId },
-        positions
-      } = action;
+      const { sourceId, positions } = action;
       return {
         ...state,
         breakpointPositions: {
           ...state.breakpointPositions,
           [sourceId]: positions
         }
       };
     }
@@ -359,16 +356,23 @@ export function getBreakpointPositions(
 export function getBreakpointPositionsForSource(
   state: OuterState,
   sourceId: string
 ): ?BreakpointPositions {
   const positions = getBreakpointPositions(state);
   return positions && positions[sourceId];
 }
 
+export function hasBreakpointPositions(
+  state: OuterState,
+  sourceId: string
+): boolean {
+  return !!getBreakpointPositionsForSource(state, sourceId);
+}
+
 export function getBreakpointPositionsForLine(
   state: OuterState,
   sourceId: string,
   line: number
 ): ?BreakpointPositions {
   const positions = getBreakpointPositionsForSource(state, sourceId);
   if (!positions) {
     return null;
--- a/devtools/client/debugger/new/src/utils/defer.js
+++ b/devtools/client/debugger/new/src/utils/defer.js
@@ -1,16 +1,22 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
 
-export default function defer() {
+type Deferred<T> = {
+  promise: Promise<T>,
+  resolve: (arg: T) => mixed,
+  reject: (arg: mixed) => mixed
+};
+
+export default function defer<T>(): Deferred<T> {
   let resolve = () => {};
   let reject = () => {};
-  const promise: Promise<any> = new Promise((_res, _rej) => {
+  const promise = new Promise((_res, _rej) => {
     resolve = _res;
     reject = _rej;
   });
 
   return { resolve, reject, promise };
 }