From 334be9f963c933f5dd068cb6bac20b84749029c8 Mon Sep 17 00:00:00 2001 From: Harry Brundage Date: Tue, 27 Oct 2020 13:26:26 -0400 Subject: [PATCH] Switch to using one tree per method instead of a map of per-method handlers on each node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes pretty printing annoying, but increases performance! With n trees instead of one tree, each tree is only split for handlers it actually has, so for HTTP verbs like POST or PUT that tend to have fewer routes, the trees are smaller and faster to traverse. For the HTTP GET tree, there are fewer nodes and I think better cache locality as that tree is traversed the most often. Each verb doesn't pay any traversal penalty for the other trees' size. This also results in more instances of more selective version stores, which means traversing them should be faster at the expense of a bit more memory consumption. This also makes the constraint implementation (see #166) easier, and prevents bugs like #132, and avoids the extra checks we have to do to fix that bug. This also prevents tree traversal for methods where there are no routes at all, which is a small optimization but kinda nice regardless. For the pretty printing algorithm, I think a nice pretty print wouldn't be per method and would instead show all routes in the same list, so I added code to merge the separate node trees and then pretty print the merged tree! To make it look pretty I added some "compression" to the tree where branches that only had one branch get compressed down, which if you ask me results in some prettier output, see the tests. Benchmarks: ``` kamloop ~/C/find-my-way (master) ➜ npm run bench; git checkout one-tree-per-method; npm run bench > find-my-way@3.0.4 bench /Users/airhorns/Code/find-my-way > node bench.js lookup static route x 42,774,309 ops/sec ±0.84% (580 runs sampled) lookup dynamic route x 3,536,084 ops/sec ±0.70% (587 runs sampled) lookup dynamic multi-parametric route x 1,842,343 ops/sec ±0.92% (587 runs sampled) lookup dynamic multi-parametric route with regex x 1,477,768 ops/sec ±0.57% (590 runs sampled) lookup long static route x 3,350,884 ops/sec ±0.62% (589 runs sampled) lookup long dynamic route x 2,491,556 ops/sec ±0.63% (585 runs sampled) lookup static versioned route x 9,241,735 ops/sec ±0.44% (586 runs sampled) find static route x 36,660,039 ops/sec ±0.76% (581 runs sampled) find dynamic route x 4,473,753 ops/sec ±0.72% (588 runs sampled) find dynamic multi-parametric route x 2,202,207 ops/sec ±1.00% (578 runs sampled) find dynamic multi-parametric route with regex x 1,680,101 ops/sec ±0.76% (579 runs sampled) find long static route x 4,633,069 ops/sec ±1.04% (588 runs sampled) find long dynamic route x 3,333,916 ops/sec ±0.76% (586 runs sampled) find static versioned route x 10,779,325 ops/sec ±0.73% (586 runs sampled) find long nested dynamic route x 1,379,726 ops/sec ±0.45% (587 runs sampled) find long nested dynamic route with other method x 1,962,454 ops/sec ±0.97% (587 runs sampled) > find-my-way@3.0.4 bench /Users/airhorns/Code/find-my-way > node bench.js lookup static route x 41,200,005 ops/sec ±0.98% (591 runs sampled) lookup dynamic route x 3,553,160 ops/sec ±0.28% (591 runs sampled) lookup dynamic multi-parametric route x 2,047,064 ops/sec ±0.83% (584 runs sampled) lookup dynamic multi-parametric route with regex x 1,500,267 ops/sec ±0.64% (590 runs sampled) lookup long static route x 3,406,235 ops/sec ±0.77% (588 runs sampled) lookup long dynamic route x 2,338,285 ops/sec ±1.60% (589 runs sampled) lookup static versioned route x 9,239,314 ops/sec ±0.40% (586 runs sampled) find static route x 35,230,842 ops/sec ±0.92% (578 runs sampled) find dynamic route x 4,469,776 ops/sec ±0.33% (590 runs sampled) find dynamic multi-parametric route x 2,237,214 ops/sec ±1.39% (585 runs sampled) find dynamic multi-parametric route with regex x 1,533,243 ops/sec ±1.04% (581 runs sampled) find long static route x 4,585,833 ops/sec ±0.51% (588 runs sampled) find long dynamic route x 3,491,155 ops/sec ±0.45% (589 runs sampled) find static versioned route x 10,801,810 ops/sec ±0.89% (580 runs sampled) find long nested dynamic route x 1,418,610 ops/sec ±0.68% (588 runs sampled) find long nested dynamic route with other method x 2,499,722 ops/sec ±0.38% (587 runs sampled) ``` --- index.js | 94 ++++++++++++++++++++++++----------- lib/pretty-print.js | 83 +++++++++++++++++++++++++++++++ node.js | 102 ++++++++------------------------------ test/pretty-print.test.js | 36 ++++++-------- 4 files changed, 185 insertions(+), 130 deletions(-) create mode 100644 lib/pretty-print.js diff --git a/index.js b/index.js index f45755c0..27141ad7 100644 --- a/index.js +++ b/index.js @@ -15,6 +15,7 @@ const assert = require('assert') const http = require('http') const fastDecode = require('fast-decode-uri-component') const isRegexSafe = require('safe-regex2') +const { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode } = require('./lib/pretty-print') const Node = require('./node') const NODE_TYPES = Node.prototype.types const httpMethods = http.METHODS @@ -26,6 +27,18 @@ if (!isRegexSafe(FULL_PATH_REGEXP)) { const acceptVersionStrategy = require('./lib/accept-version') +function buildMethodMap () { + const code = [] + for (var i = 0; i < http.METHODS.length; i++) { + var m = http.METHODS[i] + code.push(`this['${m}'] = null`) + } + return new Function(code.join('\n')) // eslint-disable-line +} + +// Object with prototype slots for all the HTTP methods +const MethodMap = buildMethodMap() + function Router (opts) { if (!(this instanceof Router)) { return new Router(opts) @@ -51,7 +64,7 @@ function Router (opts) { this.maxParamLength = opts.maxParamLength || 100 this.allowUnsafeRegex = opts.allowUnsafeRegex || false this.versioning = opts.versioning || acceptVersionStrategy - this.tree = new Node({ versions: this.versioning.storage() }) + this.trees = new MethodMap() this.routes = [] } @@ -195,7 +208,6 @@ Router.prototype._on = function _on (method, path, opts, handler, store) { Router.prototype._insert = function _insert (method, path, kind, params, handler, store, regex, version) { const route = path - var currentNode = this.tree var prefix = '' var pathLen = 0 var prefixLen = 0 @@ -203,6 +215,12 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler var max = 0 var node = null + var currentNode = this.trees[method] + if (currentNode === null) { + currentNode = new Node({ method: method, versions: this.versioning.storage() }) + this.trees[method] = currentNode + } + while (true) { prefix = currentNode.prefix prefixLen = prefix.length @@ -218,10 +236,11 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler if (len < prefixLen) { node = new Node( { + method: method, prefix: prefix.slice(len), children: currentNode.children, kind: currentNode.kind, - handlers: new Node.Handlers(currentNode.handlers), + handler: currentNode.handler, regex: currentNode.regex, versions: currentNode.versions } @@ -239,15 +258,16 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler // the handler should be added to the current node, to a child otherwise if (len === pathLen) { if (version) { - assert(!currentNode.getVersionHandler(version, method), `Method '${method}' already declared for route '${route}' version '${version}'`) - currentNode.setVersionHandler(version, method, handler, params, store) + assert(!currentNode.getVersionHandler(version), `Method '${method}' already declared for route '${route}' version '${version}'`) + currentNode.setVersionHandler(version, handler, params, store) } else { - assert(!currentNode.getHandler(method), `Method '${method}' already declared for route '${route}'`) - currentNode.setHandler(method, handler, params, store) + assert(!currentNode.handler, `Method '${method}' already declared for route '${route}'`) + currentNode.setHandler(handler, params, store) } currentNode.kind = kind } else { node = new Node({ + method: method, prefix: path.slice(len), kind: kind, handlers: null, @@ -255,9 +275,9 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler versions: this.versioning.storage() }) if (version) { - node.setVersionHandler(version, method, handler, params, store) + node.setVersionHandler(version, handler, params, store) } else { - node.setHandler(method, handler, params, store) + node.setHandler(handler, params, store) } currentNode.addChild(node) } @@ -275,11 +295,11 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler continue } // there are not children within the given label, let's create a new one! - node = new Node({ prefix: path, kind: kind, handlers: null, regex: regex, versions: this.versioning.storage() }) + node = new Node({ method: method, prefix: path, kind: kind, regex: regex, versions: this.versioning.storage() }) if (version) { - node.setVersionHandler(version, method, handler, params, store) + node.setVersionHandler(version, handler, params, store) } else { - node.setHandler(method, handler, params, store) + node.setHandler(handler, params, store) } currentNode.addChild(node) @@ -287,11 +307,11 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler // the node already exist } else if (handler) { if (version) { - assert(!currentNode.getVersionHandler(version, method), `Method '${method}' already declared for route '${route}' version '${version}'`) - currentNode.setVersionHandler(version, method, handler, params, store) + assert(!currentNode.getVersionHandler(version), `Method '${method}' already declared for route '${route}' version '${version}'`) + currentNode.setVersionHandler(version, handler, params, store) } else { - assert(!currentNode.getHandler(method), `Method '${method}' already declared for route '${route}'`) - currentNode.setHandler(method, handler, params, store) + assert(!currentNode.handler, `Method '${method}' already declared for route '${route}'`) + currentNode.setHandler(handler, params, store) } } return @@ -299,7 +319,7 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler } Router.prototype.reset = function reset () { - this.tree = new Node({ versions: this.versioning.storage() }) + this.trees = new MethodMap() this.routes = [] } @@ -358,6 +378,9 @@ Router.prototype.lookup = function lookup (req, res, ctx) { } Router.prototype.find = function find (method, path, version) { + var currentNode = this.trees[method] + if (currentNode === null) return null + if (path.charCodeAt(0) !== 47) { // 47 is '/' path = path.replace(FULL_PATH_REGEXP, '/') } @@ -370,7 +393,6 @@ Router.prototype.find = function find (method, path, version) { } var maxParamLength = this.maxParamLength - var currentNode = this.tree var wildcardNode = null var pathLenWildcard = 0 var decoded = null @@ -388,8 +410,8 @@ Router.prototype.find = function find (method, path, version) { // found the route if (pathLen === 0 || path === prefix) { var handle = version === undefined - ? currentNode.handlers[method] - : currentNode.getVersionHandler(version, method) + ? currentNode.handler + : currentNode.getVersionHandler(version) if (handle !== null && handle !== undefined) { var paramsObj = {} if (handle.paramsLength > 0) { @@ -419,13 +441,13 @@ Router.prototype.find = function find (method, path, version) { } var node = version === undefined - ? currentNode.findChild(path, method) - : currentNode.findVersionChild(version, path, method) + ? currentNode.findChild(path) + : currentNode.findVersionChild(version, path) if (node === null) { node = currentNode.parametricBrother if (node === null) { - return this._getWildcardNode(wildcardNode, method, originalPath, pathLenWildcard) + return this._getWildcardNode(wildcardNode, originalPath, pathLenWildcard) } var goBack = previousPath.charCodeAt(0) === 47 ? previousPath : '/' + previousPath @@ -448,7 +470,7 @@ Router.prototype.find = function find (method, path, version) { // static route if (kind === NODE_TYPES.STATIC) { // if exist, save the wildcard child - if (currentNode.wildcardChild !== null && currentNode.wildcardChild.handlers[method] !== null) { + if (currentNode.wildcardChild !== null) { wildcardNode = currentNode.wildcardChild pathLenWildcard = pathLen } @@ -457,11 +479,11 @@ Router.prototype.find = function find (method, path, version) { } if (len !== prefixLen) { - return this._getWildcardNode(wildcardNode, method, originalPath, pathLenWildcard) + return this._getWildcardNode(wildcardNode, originalPath, pathLenWildcard) } // if exist, save the wildcard child - if (currentNode.wildcardChild !== null && currentNode.wildcardChild.handlers[method] !== null) { + if (currentNode.wildcardChild !== null) { wildcardNode = currentNode.wildcardChild pathLenWildcard = pathLen } @@ -545,7 +567,7 @@ Router.prototype.find = function find (method, path, version) { } } -Router.prototype._getWildcardNode = function (node, method, path, len) { +Router.prototype._getWildcardNode = function (node, path, len) { if (node === null) return null var decoded = fastDecode(path.slice(-len)) if (decoded === null) { @@ -553,7 +575,7 @@ Router.prototype._getWildcardNode = function (node, method, path, len) { ? this._onBadUrl(path.slice(-len)) : null } - var handle = node.handlers[method] + var handle = node.handler if (handle !== null && handle !== undefined) { return { handler: handle.handler, @@ -585,7 +607,21 @@ Router.prototype._onBadUrl = function (path) { } Router.prototype.prettyPrint = function () { - return this.tree.prettyPrint('', true) + const root = { + prefix: '/', + nodes: [], + children: {} + } + + for (const node of Object.values(this.trees)) { + if (node) { + flattenNode(root, node) + } + } + + compressFlattenedNode(root) + + return prettyPrintFlattenedNode(root, '', true) } for (var i in http.METHODS) { diff --git a/lib/pretty-print.js b/lib/pretty-print.js new file mode 100644 index 00000000..16287dd1 --- /dev/null +++ b/lib/pretty-print.js @@ -0,0 +1,83 @@ +function prettyPrintFlattenedNode (flattenedNode, prefix, tail) { + var paramName = '' + var methods = new Set(flattenedNode.nodes.map(node => node.method)) + + if (flattenedNode.prefix.includes(':')) { + flattenedNode.nodes.forEach((node, index) => { + var params = node.handler.params + var param = params[params.length - 1] + if (methods.size > 1) { + if (index === 0) { + paramName += param + ` (${node.method})\n` + return + } + paramName += prefix + ' :' + param + ` (${node.method})` + paramName += (index === methods.size - 1 ? '' : '\n') + } else { + paramName = params[params.length - 1] + ` (${node.method})` + } + }) + } else if (methods.size) { + paramName = ` (${Array.from(methods).join('|')})` + } + + var tree = `${prefix}${tail ? '└── ' : '├── '}${flattenedNode.prefix}${paramName}\n` + + prefix = `${prefix}${tail ? ' ' : '│ '}` + const labels = Object.keys(flattenedNode.children) + for (var i = 0; i < labels.length; i++) { + const child = flattenedNode.children[labels[i]] + tree += prettyPrintFlattenedNode(child, prefix, i === (labels.length - 1)) + } + return tree +} + +function flattenNode (flattened, node) { + if (node.handler) { + flattened.nodes.push(node) + } + + if (node.children) { + for (const child of Object.values(node.children)) { + const childPrefixSegments = child.prefix.split(/(?=\/)/) // split on the slash separator but use a regex to lookahead and not actually match it, preserving it in the returned string segments + let cursor = flattened + let parent + for (const segment of childPrefixSegments) { + parent = cursor + cursor = cursor.children[segment] + if (!cursor) { + cursor = { + prefix: segment, + nodes: [], + children: {} + } + parent.children[segment] = cursor + } + } + + flattenNode(cursor, child) + } + } +} + +function compressFlattenedNode (flattenedNode) { + const childKeys = Object.keys(flattenedNode.children) + if (flattenedNode.nodes.length === 0 && childKeys.length === 1) { + const child = flattenedNode.children[childKeys[0]] + if (child.nodes.length <= 1) { + compressFlattenedNode(child) + flattenedNode.nodes = child.nodes + flattenedNode.prefix += child.prefix + flattenedNode.children = child.children + return flattenedNode + } + } + + for (const key of Object.keys(flattenedNode.children)) { + compressFlattenedNode(flattenedNode.children[key]) + } + + return flattenedNode +} + +module.exports = { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode } diff --git a/node.js b/node.js index a38088cc..cb1f6272 100644 --- a/node.js +++ b/node.js @@ -1,8 +1,6 @@ 'use strict' const assert = require('assert') -const http = require('http') -const Handlers = buildHandlers() const types = { STATIC: 0, @@ -14,14 +12,14 @@ const types = { } function Node (options) { - // former arguments order: prefix, children, kind, handlers, regex, versions options = options || {} this.prefix = options.prefix || '/' this.label = this.prefix[0] + this.method = options.method // just for debugging and error messages this.children = options.children || {} this.numberOfChildren = Object.keys(this.children).length this.kind = options.kind || this.types.STATIC - this.handlers = new Handlers(options.handlers) + this.handler = options.handler this.regex = options.regex || null this.wildcardChild = null this.parametricBrother = null @@ -102,7 +100,7 @@ Node.prototype.reset = function (prefix, versions) { this.prefix = prefix this.children = {} this.kind = this.types.STATIC - this.handlers = new Handlers() + this.handler = null this.numberOfChildren = 0 this.regex = null this.wildcardChild = null @@ -114,57 +112,57 @@ Node.prototype.findByLabel = function (path) { return this.children[path[0]] } -Node.prototype.findChild = function (path, method) { +Node.prototype.findChild = function (path) { var child = this.children[path[0]] - if (child !== undefined && (child.numberOfChildren > 0 || child.handlers[method] !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.handler !== null)) { if (path.slice(0, child.prefix.length) === child.prefix) { return child } } child = this.children[':'] - if (child !== undefined && (child.numberOfChildren > 0 || child.handlers[method] !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.handler !== null)) { return child } child = this.children['*'] - if (child !== undefined && (child.numberOfChildren > 0 || child.handlers[method] !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.handler !== null)) { return child } return null } -Node.prototype.findVersionChild = function (version, path, method) { +Node.prototype.findVersionChild = function (version, path) { var child = this.children[path[0]] - if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version, method) !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version) !== null)) { if (path.slice(0, child.prefix.length) === child.prefix) { return child } } child = this.children[':'] - if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version, method) !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version) !== null)) { return child } child = this.children['*'] - if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version, method) !== null)) { + if (child !== undefined && (child.numberOfChildren > 0 || child.getVersionHandler(version) !== null)) { return child } return null } -Node.prototype.setHandler = function (method, handler, params, store) { +Node.prototype.setHandler = function (handler, params, store) { if (!handler) return assert( - this.handlers[method] !== undefined, - `There is already an handler with method '${method}'` + !this.handler, + `There is already an handler with method '${this.method}'` ) - this.handlers[method] = { + this.handler = { handler: handler, params: params, store: store || null, @@ -172,80 +170,24 @@ Node.prototype.setHandler = function (method, handler, params, store) { } } -Node.prototype.setVersionHandler = function (version, method, handler, params, store) { +Node.prototype.setVersionHandler = function (version, handler, params, store) { if (!handler) return - const handlers = this.versions.get(version) || new Handlers() assert( - handlers[method] === null, - `There is already an handler with version '${version}' and method '${method}'` + !this.versions.get(version), + `There is already an handler with version '${version}' and method '${this.method}'` ) - handlers[method] = { + this.versions.set(version, { handler: handler, params: params, store: store || null, paramsLength: params.length - } - this.versions.set(version, handlers) -} - -Node.prototype.getHandler = function (method) { - return this.handlers[method] -} - -Node.prototype.getVersionHandler = function (version, method) { - var handlers = this.versions.get(version) - return handlers === null ? handlers : handlers[method] -} - -Node.prototype.prettyPrint = function (prefix, tail) { - var paramName = '' - var handlers = this.handlers || {} - var methods = Object.keys(handlers).filter(method => handlers[method] && handlers[method].handler) - - if (this.prefix === ':') { - methods.forEach((method, index) => { - var params = this.handlers[method].params - var param = params[params.length - 1] - if (methods.length > 1) { - if (index === 0) { - paramName += param + ` (${method})\n` - return - } - paramName += prefix + ' :' + param + ` (${method})` - paramName += (index === methods.length - 1 ? '' : '\n') - } else { - paramName = params[params.length - 1] + ` (${method})` - } - }) - } else if (methods.length) { - paramName = ` (${methods.join('|')})` - } - - var tree = `${prefix}${tail ? '└── ' : '├── '}${this.prefix}${paramName}\n` - - prefix = `${prefix}${tail ? ' ' : '│ '}` - const labels = Object.keys(this.children) - for (var i = 0; i < labels.length - 1; i++) { - tree += this.children[labels[i]].prettyPrint(prefix, false) - } - if (labels.length > 0) { - tree += this.children[labels[labels.length - 1]].prettyPrint(prefix, true) - } - return tree + }) } -function buildHandlers (handlers) { - var code = `handlers = handlers || {} - ` - for (var i = 0; i < http.METHODS.length; i++) { - var m = http.METHODS[i] - code += `this['${m}'] = handlers['${m}'] || null - ` - } - return new Function('handlers', code) // eslint-disable-line +Node.prototype.getVersionHandler = function (version) { + return this.versions.get(version) } module.exports = Node -module.exports.Handlers = Handlers diff --git a/test/pretty-print.test.js b/test/pretty-print.test.js index dd7dd82c..8ef70e92 100644 --- a/test/pretty-print.test.js +++ b/test/pretty-print.test.js @@ -36,10 +36,8 @@ test('pretty print - parametric routes', t => { const expected = `└── / ├── test (GET) - │ └── / - │ └── :hello (GET) - └── hello/ - └── :world (GET) + │ └── /:hello (GET) + └── hello/:world (GET) ` t.is(typeof tree, 'string') @@ -57,12 +55,11 @@ test('pretty print - mixed parametric routes', t => { const tree = findMyWay.prettyPrint() - const expected = `└── / - └── test (GET) - └── / - └── :hello (GET) - :hello (POST) - └── /world (GET) + const expected = `└── /test (GET) + └── / + └── :hello (GET) + :hello (POST) + └── /world (GET) ` t.is(typeof tree, 'string') @@ -81,10 +78,8 @@ test('pretty print - wildcard routes', t => { const expected = `└── / ├── test (GET) - │ └── / - │ └── * (GET) - └── hello/ - └── * (GET) + │ └── /* (GET) + └── hello/* (GET) ` t.is(typeof tree, 'string') @@ -102,13 +97,12 @@ test('pretty print - parametric routes with same parent and followed by a static const tree = findMyWay.prettyPrint() - const expected = `└── / - └── test (GET) - └── /hello - ├── / - │ └── :id (GET) - │ :id (POST) - └── world (GET) + const expected = `└── /test (GET) + └── /hello + ├── / + │ └── :id (GET) + │ :id (POST) + └── world (GET) ` t.is(typeof tree, 'string')