From df55b9c3c5e5faff0261e0422526d8b1b220db0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 8 Jul 2019 20:40:13 +0100 Subject: [PATCH] graph: canoncialize node and edge iteration order This means that we no longer need to expose methods for extracting the order from serialized JSON. We can always count on iterating over the nodes and edges in sorted order. Test plan: `yarn test`; tests updated. --- src/core/graph.js | 88 ++++++++++++++++++--------------------- src/core/graph.test.js | 57 ++++++++++++++++--------- src/core/pagerankGraph.js | 24 +++-------- 3 files changed, 82 insertions(+), 87 deletions(-) diff --git a/src/core/graph.js b/src/core/graph.js index 6f80359..3ba5b81 100644 --- a/src/core/graph.js +++ b/src/core/graph.js @@ -1,7 +1,6 @@ // @flow import deepEqual from "lodash.isequal"; -import sortBy from "lodash.sortby"; import {makeAddressModule, type AddressModule} from "./address"; import {toCompat, fromCompat, type Compatible} from "../util/compat"; @@ -201,6 +200,14 @@ export opaque type GraphJSON = Compatible<{| export type ModificationCount = number; +// Internal-only type used to cache the sorted node and edge address order. +// The modification count is used as the cache key. +type CachedOrder = {| + +nodeOrder: $ReadOnlyArray, + +edgeOrder: $ReadOnlyArray, + +modificationCount: number, +|}; + export class Graph { _nodes: Map; _edges: Map; @@ -212,6 +219,7 @@ export class Graph { // check for comodification and to avoid needlessly checking // invariants. _modificationCount: ModificationCount; + _cachedOrder: CachedOrder; _invariantsLastChecked: {|+when: ModificationCount, +failure: ?string|}; constructor(): void { @@ -220,6 +228,11 @@ export class Graph { when: -1, failure: "Invariants never checked", }; + this._cachedOrder = { + nodeOrder: [], + edgeOrder: [], + modificationCount: 0, + }; this._nodes = new Map(); this._edges = new Map(); this._incidentEdges = new Map(); @@ -290,6 +303,16 @@ export class Graph { } } + _getOrder(): CachedOrder { + const modificationCount = this._modificationCount; + if (this._cachedOrder.modificationCount !== modificationCount) { + const edgeOrder = Array.from(this._edges.keys()).sort(); + const nodeOrder = Array.from(this._nodes.keys()).sort(); + this._cachedOrder = {nodeOrder, edgeOrder, modificationCount}; + } + return this._cachedOrder; + } + /** * Returns how many times the graph has been modified. * @@ -392,8 +415,9 @@ export class Graph { * prefix. See semantics of [Address.hasPrefix][1] for details. * * Clients must not modify the graph during iteration. If they do so, an - * error may be thrown at the iteration call site. The iteration order is - * undefined. + * error may be thrown at the iteration call site. + * + * Nodes are yielded in address-sorted order. * * [1]: https://github.com/sourcecred/sourcecred/blob/7c7fa2d83d4fd5ba38efb2b2f4e0244235ac1312/src/core/address.js#L74 */ @@ -411,8 +435,9 @@ export class Graph { initialModificationCount: ModificationCount, prefix: NodeAddressT ): Iterator { - for (const node of this._nodes.values()) { - if (NodeAddress.hasPrefix(node.address, prefix)) { + for (const address of this._getOrder().nodeOrder) { + if (NodeAddress.hasPrefix(address, prefix)) { + const node = NullUtil.get(this._nodes.get(address)); this._checkForComodification(initialModificationCount); this._maybeCheckInvariants(); yield node; @@ -573,8 +598,9 @@ export class Graph { * `NodeAddress.empty`, which is a prefix of every node address. * * Clients must not modify the graph during iteration. If they do so, an - * error may be thrown at the iteration call site. The iteration order is - * undefined. + * error may be thrown at the iteration call site. + * + * The edges are yielded in sorted address order. */ edges(options: EdgesOptions): Iterator { if (options == null) { @@ -606,7 +632,8 @@ export class Graph { srcPrefix: NodeAddressT, dstPrefix: NodeAddressT ): Iterator { - for (const edge of this._edges.values()) { + for (const address of this._getOrder().edgeOrder) { + const edge = NullUtil.get(this._edges.get(address)); if ( (showDangling || this.isDanglingEdge(edge.address) === false) && EdgeAddress.hasPrefix(edge.address, addressPrefix) && @@ -750,20 +777,17 @@ export class Graph { /** * Serialize a Graph into a plain JavaScript object. - * - * Runs in time `O(n log n + e)`, where `n` is the number of nodes and `e` is - * the number of edges. */ toJSON(): GraphJSON { + // Unlike Array.from(this.nodes()).map((x) => x.address), this will include + // node references. Including node references is necessary so that we can + // index edges' src and dst consistently, even for dangling edges. const sortedNodeAddresses = Array.from(this._incidentEdges.keys()).sort(); const nodeAddressToSortedIndex = new Map(); sortedNodeAddresses.forEach((address, i) => { nodeAddressToSortedIndex.set(address, i); }); - const sortedEdges = sortBy( - Array.from(this.edges({showDangling: true})), - (x) => x.address - ); + const sortedEdges = Array.from(this.edges({showDangling: true})); const indexedEdges: IndexedEdgeJSON[] = sortedEdges.map( ({src, dst, address, timestampMs}) => { const srcIndex = NullUtil.get(nodeAddressToSortedIndex.get(src)); @@ -776,7 +800,7 @@ export class Graph { }; } ); - const sortedNodes = sortBy(Array.from(this.nodes()), (x) => x.address); + const sortedNodes = Array.from(this.nodes()); const indexedNodes: IndexedNodeJSON[] = sortedNodes.map( ({address, description, timestampMs}) => { const index = NullUtil.get(nodeAddressToSortedIndex.get(address)); @@ -1120,35 +1144,3 @@ export function edgeToParts( const timestampMs = edge.timestampMs; return {addressParts, srcParts, dstParts, timestampMs}; } - -/* - * When JSON-serialized, the graph has all of the edges in sorted - * order. This makes it possible to compactly represent metadata - * associated with every edge without needing to duplicate the - * (lengthy) edge addresses. - * This method makes it possible for consumers of Graph to package - * metadata in the same way, without needing to manually re-sort the - * edges. - */ -export function sortedEdgeAddressesFromJSON( - json: GraphJSON -): $ReadOnlyArray { - const {edges} = fromCompat(COMPAT_INFO, json); - return edges.map((x) => EdgeAddress.fromParts(x.address)); -} - -/* - * When JSON-serialized, the graph has all of the nodes in sorted - * order. This makes it possible to compactly represent metadata - * associated with every node without needing to duplicate the - * (lengthy) node addresses. - * This method makes it possible for consumers of Graph to package - * metadata in the same way, without needing to manually re-sort the - * nodes. - */ -export function sortedNodeAddressesFromJSON( - json: GraphJSON -): $ReadOnlyArray { - const {sortedNodeAddresses} = fromCompat(COMPAT_INFO, json); - return sortedNodeAddresses.map((x) => NodeAddress.fromParts(x)); -} diff --git a/src/core/graph.test.js b/src/core/graph.test.js index 9febab9..db59e02 100644 --- a/src/core/graph.test.js +++ b/src/core/graph.test.js @@ -18,8 +18,6 @@ import { edgeToString, edgeToStrings, edgeToParts, - sortedEdgeAddressesFromJSON, - sortedNodeAddressesFromJSON, } from "./graph"; import {advancedGraph, node, partsNode, edge, partsEdge} from "./graphTestUtil"; @@ -382,12 +380,30 @@ describe("core/graph", () => { const graph = new Graph().addNode(src).addNode(dst); expect(graph.hasNode(src.address)).toBe(true); expect(graph.hasNode(dst.address)).toBe(true); - expect(Array.from(graph.nodes())).toEqual([src, dst]); + expect(Array.from(graph.nodes())).toEqual([dst, src]); expect(graph.node(src.address)).toEqual(src); expect(graph.node(dst.address)).toEqual(dst); }); }); + describe("node ordering", () => { + const nodes = (g) => Array.from(g.nodes()); + const sorted = (g) => sortBy(nodes(g), (x) => x.address); + it("returns nodes in sorted order, after a sequence of additions and removals", () => { + const g1 = advancedGraph().graph1(); + const g2 = advancedGraph().graph2(); + expect(sorted(g1)).toEqual(nodes(g1)); + expect(nodes(g1)).toEqual(nodes(g2)); + }); + it("returns nodes in sorted order, after deserialization", () => { + const roundTrip = (g) => Graph.fromJSON(g.toJSON()); + const g1 = roundTrip(advancedGraph().graph1()); + const g2 = roundTrip(advancedGraph().graph2()); + expect(sorted(g1)).toEqual(nodes(g1)); + expect(nodes(g1)).toEqual(nodes(g2)); + }); + }); + describe("node prefix filtering", () => { const n1 = partsNode([]); const n2 = partsNode(["foo"]); @@ -568,6 +584,24 @@ describe("core/graph", () => { }); }); + describe("edge ordering", () => { + const edges = (g) => Array.from(g.edges({showDangling: true})); + const sorted = (g) => sortBy(edges(g), (x) => x.address); + it("returns edges in sorted order, after a sequence of additions and removals", () => { + const g1 = advancedGraph().graph1(); + const g2 = advancedGraph().graph2(); + expect(sorted(g1)).toEqual(edges(g1)); + expect(edges(g1)).toEqual(edges(g2)); + }); + it("returns edges in sorted order, after deserialization", () => { + const roundTrip = (g) => Graph.fromJSON(g.toJSON()); + const g1 = roundTrip(advancedGraph().graph1()); + const g2 = roundTrip(advancedGraph().graph2()); + expect(sorted(g1)).toEqual(edges(g1)); + expect(edges(g1)).toEqual(edges(g2)); + }); + }); + describe("edges filtering", () => { const src1 = partsNode(["src", "1"]); const src2 = partsNode(["src", "2"]); @@ -1493,21 +1527,4 @@ describe("core/graph", () => { expect(edgeToParts(edge)).toEqual(expected); }); }); - - it("sortedEdgeAddressesFromJSON", () => { - const json = advancedGraph() - .graph1() - .toJSON(); - const sortedEdgeAddresses = sortedEdgeAddressesFromJSON(json); - const expected = sortedEdgeAddresses.slice().sort(); - expect(sortedEdgeAddresses).toEqual(expected); - }); - it("sortedNodeAddressesFromJSON", () => { - const json = advancedGraph() - .graph1() - .toJSON(); - const sortedNodeAddresses = sortedNodeAddressesFromJSON(json); - const expected = sortedNodeAddresses.slice().sort(); - expect(sortedNodeAddresses).toEqual(expected); - }); }); diff --git a/src/core/pagerankGraph.js b/src/core/pagerankGraph.js index 0a63122..d5cf931 100644 --- a/src/core/pagerankGraph.js +++ b/src/core/pagerankGraph.js @@ -10,8 +10,6 @@ import { type NodeAddressT, type EdgeAddressT, type GraphJSON, - sortedEdgeAddressesFromJSON, - sortedNodeAddressesFromJSON, NodeAddress, type NeighborsOptions, } from "./graph"; @@ -567,19 +565,9 @@ export class PagerankGraph { this._verifyGraphNotModified(); const graphJSON = this.graph().toJSON(); - const nodes = sortedNodeAddressesFromJSON(graphJSON).filter((x) => - this.graph().hasNode(x) - ); - const scores: number[] = nodes.map((x) => - NullUtil.get(this._scores.get(x)) - ); + const scores = Array.from(this.nodes()).map((x) => x.score); - const edgeAddresses = sortedEdgeAddressesFromJSON(graphJSON).filter( - (a) => this.graph().isDanglingEdge(a) === false - ); - const edgeWeights: EdgeWeight[] = edgeAddresses.map((x) => - NullUtil.get(this._edgeWeights.get(x)) - ); + const edgeWeights = Array.from(this.edges()).map((x) => x.weight); const toWeights: number[] = edgeWeights.map((x) => x.toWeight); const froWeights: number[] = edgeWeights.map((x) => x.froWeight); @@ -604,16 +592,14 @@ export class PagerankGraph { } = fromCompat(COMPAT_INFO, json); const graph = Graph.fromJSON(graphJSON); - const nodeAddresses = sortedNodeAddressesFromJSON(graphJSON).filter((x) => - graph.hasNode(x) - ); + const nodeAddresses = Array.from(graph.nodes()).map((x) => x.address); const scoreMap: Map = new Map(); for (let i = 0; i < nodeAddresses.length; i++) { scoreMap.set(nodeAddresses[i], scores[i]); } - const edgeAddresses = sortedEdgeAddressesFromJSON(graphJSON).filter( - (x) => graph.isDanglingEdge(x) === false + const edgeAddresses = Array.from(graph.edges({showDangling: false})).map( + (x) => x.address ); const edgeWeights: Map = new Map(); for (let i = 0; i < edgeAddresses.length; i++) {