Closed rentalhost closed 1 year ago
Hi @rentalhost , If i remember the reason right, this was implemented because most of queries doesn't have any escaping to do, and current implementation was then faster. That's needs to be checked in detail, the goal beeing to always to optimize the most common uses case.
@rusher You are right. Surprisingly String.replace(RegExp) manages to be slower than its hardcoded version.
https://jsbench.me/01lf6yjybs/1
Version | No needs to escape | Needs to escape |
---|---|---|
Original | 1.818.490 ops/s | 345.642 ops/s |
PR | 1.294.947 ops/s | 305.426 ops/s |
.test() | 567.054 ops/s | 320.359 ops/s |
This PR optimizes
escape()
by using a single replacement RegExp instead of a hardcoded replacement. It is about ~30% faster than current implementation.Regular expression ordering also prioritizes most common characters first.
I also don't see much reason to escape
"
(double quotes), since theescapeString()
function is only used in places where the wrapper is always single quotes. I kept it because other code is tested this way, but in practice, there shouldn't be any side effects.New tests added just to double-check that. All works as expected.