Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All changes for LoyaltyLion #1

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

All changes for LoyaltyLion #1

wants to merge 23 commits into from

Conversation

clarkdave
Copy link
Member

No description provided.

tsconfig.json Outdated Show resolved Hide resolved
Copy link

@tobenna tobenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏅

@clarkdave clarkdave force-pushed the loyaltylion/changes branch 2 times, most recently from e9a92b6 to 65d8e15 Compare November 29, 2018 09:59
@clarkdave clarkdave force-pushed the loyaltylion/changes branch 2 times, most recently from 7b15883 to 23d1adc Compare January 4, 2019 12:31
clarkdave pushed a commit that referenced this pull request Nov 19, 2019
clarkdave pushed a commit that referenced this pull request Nov 19, 2019
@clarkdave clarkdave force-pushed the loyaltylion/changes branch 2 times, most recently from 23e3bf7 to 6bd52e0 Compare November 19, 2019 19:46
pamdesilva pushed a commit that referenced this pull request Nov 17, 2020
)

* fix(aurora): pass formatOptions to Data API Client, fix UUID type support and all other extensions

* fix(aurora): refactored the code to avoid duplication (#1)

* fix(aurora): refactored the code to avoid duplication
clarkdave and others added 15 commits November 20, 2020 08:57
This type is used instead of a regular EntityManager in the
EntityManager.prototype.transaction callback. It extends an
EntityManager, so can be used as normal, but is not assignable to one,
so functions and services can require this type in order to assert they
receive a manager that has already started a transaction
LL specific change - our legacy services use this field to determine
if a transaction is active or not, if we don't manage it via
typeorm then it's possible legacy clients will still think they
are in a transaction when they're not
typeorm currently lets pg.pool assume that clients are always safe to
add back into the pool, even if it encountered an error while running
a query. this is /usually/ fine, because the vast majority of errors
that happen during a query do not cause the client's connection itself
to become inoperable

however, if, during a query, the client's connection is killed, the
query will fail and the client will be placed back into the pool even
though it's linked to a now broken connection, i.e. that client will
never work again, but because pg.Pool doesn't know that, it'll keep
handing it out. end result: lots of failed queries

this patches `Connection.prototype.query` so that it passes the error
back to the driver when it calls `queryRunner.release`. the driver can
(if applicable) then inspect the error and decide if it should result
in the client being removed from the pool or just released as normal
we're not confident in our ability to accurately determine what a "bad"
error (i.e. one that has rendered the connection broken). to be safe,
we'll instruct the pool to remove any client that failed to execute
a query

though this will result in more pool churn, we're using pgbouncer so
those connections are cheap, and this provides maximum safety overall
frangz and others added 6 commits November 14, 2022 15:40
Motivation: the query builder (and within it, replacePropertyNames and
associated functions) is pretty CPU intensive. For our workload, it's
one of the hottest functions in our entire stack.

While improved in typeorm#4760,
There are still outstanding issues relating to perf e.g. typeorm#3857

As we all know though, the first step in optimization is to measure
systematically ;)

https://wiki.c2.com/?ProfileBeforeOptimizing

On my machine, this benchmark runs in ~3500ms or about 0.35ms/query.
This tells us there's a way to go - on my stack, that's about 1/3 of a
typical query's latency!
Context: the query builder is pretty CPU intensive, and can be slow -
e.g. typeorm#3857

One of the things which makes this slow is `escapeRegExp` in the query
builder: we freshly construct the same RegExp once per
`replacePropertyName` invocation (many times per overall query!) and
since the RegExp itself is constant -- we can lift it out and construct
it once.

Over-all this saves about 8% on our query build times as measured by
 typeorm#8955.
Digging further into typeorm#3857.

See also typeorm#8955, typeorm#8956.

As [previously
discussed](typeorm#3857 (comment)),
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants