Improve replacePropertyNames performance#4760
Improve replacePropertyNames performance#4760imnotjames merged 3 commits intotypeorm:masterfrom mohd-akram:improve-query-perf
Conversation
|
@pleerock Could this be merged? |
|
@Kononnable @vlapo can you guys help to review this one? |
|
This seems more useful now given #5309. |
|
I also run into these code when profiling. @mohd-akram's work looks good. |
|
Any update on this? Anything blocking this improvement? |
|
This function is most prominent one in the profiling results of our services, and I guess it applies for other DB heavy code. Any speedup to this would be with significant effect. |
|
As mentioned this is proving to be a significant bottleneck given #5309 |
|
Are there tests that verify this? There's none here, but I'm guessing there's tests that verify the functionality otherwise. |
|
Rebased against master. Gonna go looking to make sure we've got tests. |
|
Oracle fails with this - doesn't in |
|
Oh.. oh wow. This passes before & fails now because NOW we're correctly escaping regular expressions.... but before we weren't. So a The failure in question:
There's a relation to an This means that we generated invalid identifiers for oracle. Adding the replacement: fixes it! |
|
Bonus win - this fixes some undesired and undefined behavior! Wonderful. |
the `databaseName` prop on the alias isn't recognized for replacement so this adds it
|
Thanks for the contribution! |
|
@imnotjames Any chance you could push this in a new release? This should be a huge improvement and I couldn't find a way to install the master branch in my project directly (unless there's a way to do that?) Thank you so much |
|
a new release will be available this week |
|
This breaks QueryBuilder from Edit: The error is: Edit2: Relevant: |
|
The lookbehind could be removed by adjusting the replacement function (see the first commit for inspiration). |
|
@imnotjames @pleerock this broke typeorm on iOS webkit (which doesn't support lookbehind, not even in iOS 14). unfortunately all of the last typeorm releases where instantly failing on my app project (via Ionic). Even if a node-environment is the most common use of typeorm, I'd think that hybrid apps are still a significant use case. i think there needs to be some way to catch at least those basic issues before merging PRs. |
|
@mohd-akram @pleerock @imnotjames is it possible to revert or add a new version of this change without the lookbehind so it can work on React native again? I'd love to help... but the regex is a little bit too complex for my knowledge... :/ |
Reduce the number of regular expressions created & strings created when replacing the property names in a statement.
|
Reverted by #7269 . Fix will be available in |
|
But what about the initial fix about replacePropertyNames performance ? Isn't it merged in anymore ? |
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!
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!
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!
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!
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 #4760, There are still outstanding issues relating to perf e.g. #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!
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!
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!
I did some profiling and found that this function was a bottleneck when using a cache. With these changes I was able to get about 5x better performance.