Merge branch 'perf/drawn-set-one-walk' into feat/drawn-set-in-view

src/app/features/galaxy-system/routing-client.spec.ts

@@ -148,12 +148,38 @@ describe('RoutingClient with a worker', () => {
     const { client, worker } = clientWithFake();
     const once = client.route(100, 104, 1.5, 8);
     const again = client.route(100, 104, 1.5, 8);
+    const widerRange = client.route(100, 104, 2.5, 8);
 
     expect(worker.requests).toHaveLength(1);
     worker.answer({ kind: 'route', requestId: worker.requests[0].requestId, route: null, neededRangePc: 4 });
 
     expect(await again).toEqual(await once);
-    expect(worker.requests).toHaveLength(1);
+    await flush();
+    // The same two stars at another range is another question.
+    expect(worker.requests.map((request) => request.rangePc)).toEqual([1.5, 2.5]);
+    worker.answer({ kind: 'route', requestId: worker.requests[1].requestId, route: null, neededRangePc: null });
+    await expect(widerRange).resolves.toEqual({ route: null, neededRangePc: null });
+    client.dispose();
+  });
+
+  // Turning the layer off and on again while the worker is busy asks for the same graph twice. Were
+  // the second to replace the first, the first's rejection would wipe the scene's record of the second.
+  it('shares a graph already on its way for the same range and the same list of drawn stars', async () => {
+    const { client, worker } = clientWithFake();
+    const drawn = Uint32Array.of(0, 1, 2);
+    const building = client.links(3, drawn);
+    const waiting = client.links(5, drawn);
+    const again = client.links(5, drawn);
+    const sameAsBuilding = client.links(3, drawn);
+
+    worker.answer({ kind: 'links', requestId: worker.requests[0].requestId, segments: new Float32Array(6) });
+    await expect(building).resolves.toHaveLength(6);
+    await expect(sameAsBuilding).resolves.toHaveLength(6);
+    await flush();
+    worker.answer({ kind: 'links', requestId: worker.requests[1].requestId, segments: new Float32Array(12) });
+    await expect(waiting).resolves.toHaveLength(12);
+    await expect(again).resolves.toHaveLength(12);
+    expect(worker.requests.map((request) => request.kind === 'links' && request.rangePc)).toEqual([3, 5]);
     client.dispose();
   });
 

src/app/features/galaxy-system/routing-client.ts

@@ -18,12 +18,6 @@ export class SupersededRequest extends Error {
 /** A request made and not yet answered: what was asked, and the promise whoever asked is holding. */
 interface Outstanding {
   readonly request: RoutingRequest;
-  /**
-   * A route question without its id, so the same route asked for twice can be recognised. Empty for
-   * a graph: the scene never asks for the same graph twice, and spelling out 70 000 drawn stars to
-   * compare costs more than the comparison could save.
-   */
-  readonly question: string;
   readonly promise: Promise<RoutingResponse>;
   readonly resolve: (response: RoutingResponse) => void;
   readonly reject: (error: Error) => void;
@@ -36,9 +30,19 @@ function outstanding(request: RoutingRequest): Outstanding {
     resolve = onResolve;
     reject = onReject;
   });
-  // eslint-disable-next-line @typescript-eslint/no-unused-vars
-  const { requestId, ...question } = request;
-  return { request, question: request.kind === 'route' ? JSON.stringify(question) : '', promise, resolve, reject };
+  return { request, promise, resolve, reject };
+}
+
+/**
+ * Whether two requests ask the same question. A graph is the same when it is for the same range and
+ * the very same list of drawn stars: the star field replaces that list whenever the set changes, so
+ * one array is one set, and comparing 70 000 indices would cost more than sharing could save.
+ */
+function asksTheSame(a: RoutingRequest, b: RoutingRequest): boolean {
+  if (a.kind === 'links' || b.kind === 'links') {
+    return a.kind === 'links' && b.kind === 'links' && a.rangePc === b.rangePc && a.drawn === b.drawn;
+  }
+  return a.fromId === b.fromId && a.toId === b.toId && a.rangePc === b.rangePc && a.ceilingPc === b.ceilingPc;
 }
 
 /** The routing worker, where this environment has one. */
@@ -50,12 +54,13 @@ function startRoutingWorker(): Worker | undefined {
  * Asks the route questions of a worker holding its own copy of the catalogue, and hands back
  * promises.
  *
- * The worker answers one request at a time and cannot drop one it has started: a jump-link graph
- * at 8 pc is seconds of work. So requests are held here and sent one by one, and while one is out,
- * only the latest of each kind waits behind it — a newer graph replaces an older one before it is
- * ever built, and the older promise is rejected with {@link SupersededRequest}. Routes go ahead of
- * graphs, being quick and asked for by a click. The same route asked for again while it is still
- * outstanding shares the answer rather than being worked out twice.
+ * The worker answers one request at a time and cannot drop one it has started: a route with no path
+ * can be seconds of work, and a graph of the drawn stars at 8 pc a few hundred milliseconds. So
+ * requests are held here and sent one by one, and while one is out, only the latest of each kind
+ * waits behind it — a newer graph replaces an older one before it is ever built, and the older
+ * promise is rejected with {@link SupersededRequest}. Routes go ahead of graphs, being quick to ask
+ * for and asked for by a click. The same question asked again while it is still outstanding shares
+ * the answer rather than being worked out twice; see `asksTheSame`.
  *
  * Where there is no worker — the unit tests' DOM has none, and a worker can fail to load or crash —
  * the same answers are worked out in place, from the index the scene already holds.
@@ -115,11 +120,13 @@ export class RoutingClient {
     if (!this.worker) {
       return new Promise((resolve) => resolve(answerRouting(this.localIndex, request)));
     }
-    const asked = outstanding(request);
-    const same = asked.question ? [this.inFlight, this.waiting[request.kind]].find((other) => other?.question === asked.question) : undefined;
+    // Shared rather than replaced: an identical request superseding the one it repeats would reject it,
+    // and whoever holds that promise would take the rejection for its own question.
+    const same = [this.inFlight, this.waiting[request.kind]].find((other) => other !== undefined && asksTheSame(other.request, request));
     if (same) {
       return same.promise;
     }
+    const asked = outstanding(request);
     this.waiting[request.kind]?.reject(new SupersededRequest());
     this.waiting[request.kind] = asked;
     this.sendNext();