From bbdc20f8cafc63e768f248213eafe65f163cb6e5 Mon Sep 17 00:00:00 2001 From: Patrick Molgaard Date: Tue, 31 May 2022 16:56:42 +0100 Subject: [PATCH] perf: partially lift matching from regexp to js (#9032) Digging further into #3857. See also #8955, #8956. As [previously discussed](https://github.com/typeorm/typeorm/issues/3857#issuecomment-699505893), the query builder currently suffers from poor performance in two ways: quadratic numbers of operations with respect to total table/column counts, and poor constant factor performance (regexps can be expensive to build/run!) The constant-factor performance is the more tractable problem: no longer quadratically looping would be a chunky rewrite of the query builder, but we can locally refactor to be a bunch cheaper in terms of regexp operations. This change cuts the benchmark time here in ~half (yay!). We achieve this by simplifying the overall replacement regexp (we don't need our column names in there, since we already have a plain object where they're the keys to match against) so compilation of that is much cheaper, plus skipping the need to `escapeRegExp` every column as a result. --- src/query-builder/QueryBuilder.ts | 40 +++++++++---------- .../multiple-joins-querybuilder.ts | 10 ++--- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/query-builder/QueryBuilder.ts b/src/query-builder/QueryBuilder.ts index 88da9bebef..83f76e0260 100644 --- a/src/query-builder/QueryBuilder.ts +++ b/src/query-builder/QueryBuilder.ts @@ -756,27 +756,27 @@ export abstract class QueryBuilder { replacements[column.propertyPath] = column.databaseName } - const replacementKeys = Object.keys(replacements) - - if (replacementKeys.length) { - statement = statement.replace( - new RegExp( - // Avoid a lookbehind here since it's not well supported - `([ =\(]|^.{0})` + - `${escapeRegExp( - replaceAliasNamePrefix, - )}(${replacementKeys - .map(escapeRegExp) - .join("|")})` + - `(?=[ =\)\,]|.{0}$)`, - "gm", - ), - (_, pre, p) => - `${pre}${replacementAliasNamePrefix}${this.escape( + statement = statement.replace( + new RegExp( + // Avoid a lookbehind here since it's not well supported + `([ =\(]|^.{0})` + // any of ' =(' or start of line + // followed by our prefix, e.g. 'tablename.' or '' + `${escapeRegExp( + replaceAliasNamePrefix, + )}([^ =\(\)\,]+)` + // a possible property name: sequence of anything but ' =(),' + // terminated by ' =),' or end of line + `(?=[ =\)\,]|.{0}$)`, + "gm", + ), + (match, pre, p) => { + if (replacements[p]) { + return `${pre}${replacementAliasNamePrefix}${this.escape( replacements[p], - )}`, - ) - } + )}` + } + return match + }, + ) } return statement diff --git a/test/benchmark/multiple-joins-querybuilder/multiple-joins-querybuilder.ts b/test/benchmark/multiple-joins-querybuilder/multiple-joins-querybuilder.ts index 7c26b6ee0a..0841f23610 100644 --- a/test/benchmark/multiple-joins-querybuilder/multiple-joins-querybuilder.ts +++ b/test/benchmark/multiple-joins-querybuilder/multiple-joins-querybuilder.ts @@ -47,11 +47,11 @@ describe("benchmark > QueryBuilder > wide join", () => { /** * On a M1 macbook air, 5 runs: - * 3501ms - * 3574ms - * 3575ms - * 3563ms - * 3567ms + * 1861ms + * 1850ms + * 1859ms + * 1859ms + * 1884ms */ }) })