perf: refactor value manipulations #375
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Kept the old code commented for ease of revert in case necessary. |
|
I will change all the |
|
I will also fix the error of course, it's a bit odd CI results appear so late. |
Thanks ! CI is not triggered automatically on patch branches, we have to launch it manually. |
|
Ah, that makes sense |
I can see them from the actions tab, thanks! |
|
I really appreciate your work buddy! Do you have these benchmarks somewhere to compare performance between the main branch and yours? I am thinking if it will actually make much difference. I know that in general classic for loops are faster than built in array methods but the difference can be observed mainly when working with datasets that contain milions of rows. The optimizations made by @vejja for the cheerio made sense because they were traversing through the generated HTML many times which was decreasing the performance but in the case of your PR it may not even be noticable. Unless there is a benchmark that proves I am wrong :) |
|
@Baroshem I have indeed checked every update I pushed with multiple benchmarks, both on node and on browsers. You're correct it may not be that significant in the short term compared to the cheerio update, but it's an opotimization that will add up at runtime in the long run, benefitting not only the module, but also the users' applications in terms of performance. I can attach some benchmark results if you'd like to see the difference it made. |
|
Yes please do so! We will take a look at it with @vejja :) |
|
Sure thing! |
|
@Baroshem Heya! I really apologize for the long disappearance, got caught up with personal life and haven't had much time to work on this, I have the benchmark results here: Here's the benchmark code (ran separately to avoid any sort of engine level caching): const bigObject = {}
for(let i = 0; i < 1; i++){
bigObject[i] = i+1
}
console.time("bigObjectAsArray")
for (let i = 0; i < 1; i++) {
const bigObjectAsArray = Object.entries(bigObject).map(([key, value]) => [value, key])
}
console.timeEnd("bigObjectAsArray")
console.time("bigObjectAsObject")
for (let i = 0; i < 1; i++) {
const bigObjectAsArray = []
for(const key in bigObject){
bigObjectAsArray.push([bigObject[key], key])
}
}
console.timeEnd("bigObjectAsObject")As you can see, there is a very clear performance difference even on a single iteration in a single test loops. This differs further as the objects become larger and ran through more iterations. |
|
I will also refactor this PR in a new one that I will open soon, because I clearly messed something up right here. |
|
To be very honest (and I'm not sure you'll like it that much), I am not convinced that we should sacrifice code stability for marginal optimization at this stage of the project. There are many core features in the module that are probably going to change significantly (I'm thinking integrating runtime hook events within the core radix router), and I think it would be safer to stabilize these parts of the code before undergoing loop optimization. Although you're right on single line performance timings, we don't loop through that many objects and our objects are not large at all, so overall the impact is not even going to be noticeable. However, there is one key part of the code where we would really need help in performance/optimization, which is the Cheerio parser. The performance gains are likely to be huge, so if you would accept to have a look at it I think it would be truly fantastic. Would you have a look at issue #341 ? OP reports that response time went from 14ms to 200-500ms. So if we were able to bring it back down into the 20-50ms range, that would be an amazing 10x performance gain ! In addition we also got reports of CPU/memory crashes in Cloudflare Workers. We know exactly what the issue is: Cheerio is very very slow. And we also know what the solution is: use regexes instead of Cheerio. This analysis is based on the fact that OP is comparing Nuxt-Security v0.13 (which used regexes) with Nuxt-Security v1.0 (which uses Cheerio). Would you by any chance happen to be comfortable with regexes? Let me know your feedback - sorry to be a bit directive here but I think your focus on performance would be highly beneficial to the module. |
|
First of all, thanks for the amazing work! You really did a lot of work to benchmark the performance. But I have to agree on @vejja on this one. I would prefer not to change so many things in the code to achieve such small performance improvement. It is noticable but changing so much can result in unwanted bugs that I would prefer to avoid at all. But as Vejja mentioned, there are for sure opportunities to significantly improve the prrformance without actually rewriting the code of the module like the example with cheerio. If you would like to cintribute, I am sire this is a great place to start. It would also allow to remove dependency from the module. This would help the module a lot! |
|
@vejja @Baroshem |
|
Closing in favor of #404 by @GalacticHypernova |
Types of changes
Description
Having many manipulations (especially when unnecessary) can be really detrimental to performance. This PR aims to refactor, wherever possible, said iterations.
Checklist:
Not necessary as it doesn't affect functionality.