feat(util): use fast-querystring for building url #1673
Conversation
anonrig
force-pushed
the
perf/querystring
branch
from
5df7b90
to
f9971e7
Compare
|
Destructuring fast-querystring and calling it using
|
Codecov ReportBase: 94.74% // Head: 94.73% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
==========================================
- Coverage 94.74% 94.73% -0.02%
==========================================
Files 53 53
Lines 4892 4877 -15
==========================================
- Hits 4635 4620 -15
Misses 257 257
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@mcollina wdyt? Another external dependency? |
|
@ronag Kinda the opposite of that, it is now less strict and accepts wider range of params. When queryParams were first implemented, I remember we already talked how we may potentially revise restrictions to be more lenient in the future, so I guess now's that moment :D |
|
There are very minor differences between undici's current implementation and fast-querystring. These include:
Implementing these minor changes, the performance of the current impl. is faster/on par with fast-querystring. diff --git a/lib/core/util.js b/lib/core/util.js
index e9a8384..d3fe468 100644
--- a/lib/core/util.js
+++ b/lib/core/util.js
@@ -44,9 +44,13 @@ function buildURL (url, queryParams) {
throw new Error('Query params must be an object')
}
- const parts = []
- for (let [key, val] of Object.entries(queryParams)) {
- if (val === null || typeof val === 'undefined') {
+ let parts = ''
+ const keys = Object.keys(queryParams)
+ for (let i = 0; i < keys.length; i++) {
+ let key = keys[i] // eslint-disable-line prefer-const
+ let val = queryParams[key]
+
+ if (val == null) {
continue
}
@@ -54,18 +58,19 @@ function buildURL (url, queryParams) {
val = [val]
}
+ const isLast = i === keys.length - 1
+
for (const v of val) {
if (isObject(v)) {
throw new Error('Passing object as a query param is not supported, please serialize to string up-front')
}
- parts.push(encode(key) + '=' + encode(v))
+ parts += encode(key) + '='
+ parts += encode(v) + (isLast ? '' : '&')
}
}
- const serializedParams = parts.join('&')
-
- if (serializedParams) {
- url += '?' + serializedParams
+ if (parts) {
+ url += '?' + parts
}
return urlHowever I also noticed that the benchmark never gives the same results between results. One run might have undici destroy fast-querystring, and another run will show the exact opposite. However the perf gains from the modified impl. destroy the current one in undici. |
|
@KhafraDev Could you open an alternative PR with these changes? |
|
@KhafraDev Your code change does not reflect the truth, since it is not compliant with
Can you share with us the updated benchmarks after these changes? I'm curious how it will be different from the implementation of In either scenario, I'm thankful for your comment/contribution. |
|
@KhafraDev Can you also share your input for the benchmarks? Here's |
|
The idea I was going for was to match undici's current behavior, rather than mimicking fast-querystring's. It would take slightly more work in the future to add support for bigints (etc.) which seemed out of scope for the current pr (improving perf). The & getting appended to the last value is a mistake on my part, I've amended it 👍. Here's an update benchmark for the changes; as mentioned previously the benchmark results skew heavily between runs. edit: thought I should mention, but none of undici's benchmarks use a query - meaning the biggest perf gain was likely from cronometro itself and maybe how fast Object.entries is to Object.keys. When passing a query, fast-querystring is much faster than undici (where the biggest bottleneck comes from encodeURIComponent) when using a slightly more modified buildURL than above that handles elements that aren't an array. |
|
Just a remark: When you run your benchmarks you should imho set in your cpu governor all cpu cores to performance mode. Your benchmark tolerance is imho too high. It should be about 1-2%. Tolerance of 18 % indicates to me that your cpu cores are in on-demand mode and need to "spin up". So your benchmark is kind of tainted because e.g. your cpu starts with e.g. 800 Mhz and then goes up to 3 Ghz while benchmarking and that results in the bigger tolerance. But that is only a remark and maybe your benchmarks are valid. |
This is the main scenario that we should be comparing, TBH. And it feels like we should add it to our benchmark. |
You're correct @kibertoad Can you direct me to the correct place in the code to define a complex query-string that directly triggers |
|
@anonrig Main place that calls and not like this: So we need to add cases like that into https://github.com/nodejs/undici/blob/main/benchmarks/benchmark.js |
Unfortunately stuck using wsl. However I don't think any benchmarks here are necessarily valid regardless. Since the benchmarks do not ever call |
|
@KhafraDev considering that it is faster at some cases and slower at none, and also reduces complexity on undici side, isn't that a net positive? |
|
I'm personally for using as few third party dependencies as possible, especially in cases where it's feasible to make such an area in the code more performant without it being complex. I don't consider my modifications complex - it was done within maybe 5-10 minutes and is still understandable imo (feel free to disagree here). However, I decided to run the benchmarks in fast-querystring and found that if the object being stringified only consists of strings and arrays, node's builtin querystring is actually on par with fast-querystring (the benchmarks vary with one never being 10% slower than the other). For undici's current behavior of only accepting strings and arrays, it seems like the better choice is to use querystring instead of adding another dependency. const value = {
frappucino: "muffin",
goat: "scone",
pond: "moose",
foo: ["bar", "baz", "bal"],
bool: `${true}`,
bigIntKey: BigInt(100).toString(),
numberKey: `${256}`,
};Object modified from the fast-qs benchmark to only contain string and an array. p.s. the modified buildURL came in third place in all attempts, but isn't that important to include. |
|
@KhafraDev built-in querystring is deprecated (for reasons that I do not understand), should we be depending on that? |
|
@kibertoad |
|
True, which still means "discouraged to use": |
|
|
|
personally I'm +1 on fast-querystring |
|
If fast-querystring is only negligable faster then I don't see any reason to use it over querystring. |
|
It seems that majority opinion is in favour of switching to querystring |
|
@KhafraDev what about booleans and numbers? undici accepts these just fine too, and they are quite common |
|
Same result. Both are nearly equally fast. I can post benchmark results if you'd like. |
|
Thank you for all the comments. I was on my way to Ireland and just had the time to review and read the responses. From my perspective, here are the steps we need to take in order to improve
Even though, I don't agree with some of the comments made into this pull request, I think that we are on the correct path to improve the existing implementation. PS: I didn't want to update and replace this pull request with a |
Solves #1672.
Important Note: Even though none of the benchmarks focus on query-string building (can test it with just adding
new Error('hello')inside buildURL function), the results of the benchmarks show significant degregation in ops/sec. My educated guess would be: Due to requiring & resolving a new package, and howcronometrocreates an isolated v8 environment every time a benchmark is run, the require is never cached, and therefore the amount of time requiring thefast-querystringpackage is also included in the benchmark and as a result creates a false outcome.