Performance optimization opportunity

[prashantpalikhe]

Version

nuxt-security: 1.0.0
nuxt: 3.9.1

Reproduction Link

https://stackblitz.com/edit/github-3cbcjk

Steps to reproduce

Go to the StackBlitz, and run npm run build && npm run preview.

And, reload the page.

By default, the app is not using the nuxt-security module. In server log, you will see how long it takes for Nitro to render the app. It averages just ~1ms for me.

To simulate a more real-life page, I am rendering 5000 elements on the page.

Now, enable the nuxt-security module and repeat the steps.

You will see that the Nitro render now averages a lot more now.

What is Expected?

The security plugins should not impact the Nitro render time that drastically and should scale well with the size of the page.

What is actually happening?

This seems to be happening in the Nitro plugins that iterate through the HTML context a few number of times using cheerio to get the elements to modify.

Maybe, we can improve this by reducing the amount of times we loop over the HTML context and making changes as efficiently as possible?

I noticed this issue on a very chunky page. So not sure how much this translates to real world pages. But, nevertheless thought it might be worth investigating if we can improve the performance here.

[Baroshem]

Hey @prashantpalikhe

Thanks for this issue and reasearch!

Yes, the module shouldnt decrease the performance. We have not yet receive any issues about performance but we can look at your recommendation and try to make it work.

@vejja What are your thougths on that?

[vejja]

I'll try to measure which part of the HTML parsing is taking most time to find out if there are low-hanging fruits somewhere
We can probably leverage the event.context.security object of #298 to cache some parsed objects there

[unr]

Interestingly @Baroshem after releasing 1.0.1 from #335 our Page crashed in Cloudflare due to exceeding CPU Limits.

It appears that simply upgrading the nuxt-security module with no other changes we went from averaging 14ms responses to 200ms - 500ms on average.

With Nuxt-security 1.0.1 shipped

You can see the spike in the graph immediately after deploy at 13:25.

Reverting to nuxt-security 0.13.0

We can see the cpu response time drops back down when deployed around 14:00

[prashantpalikhe]

@Baroshem @vejja

For us, the biggest bottlenecks were the nitro plugins for the SRI and CSP nonce, specifically the body section. Since that can be quite huge.

We use web components with Declarative Shadow DOM. That means the styles for the components are repeated in HTML per usage of a component. That is how it is right now with web components that are SSR-ed. But I can imagine, there are many other ways in which HTML can grow in size. The use of inline SVG icons comes to mind.

So as a workaround, we disabled nonce and contentSecurityPolicy in nuxt-security and we handle it ourselves. Where we do everything like nuxt-security does except we ignore the body section. That saves us 200-300ms in server response time.

I don't know how we can fix this in the module itself. Maybe a configuration option to set which sections to look into when setting nonce could be useful.

But that is hiding the issue, not really solving it.

[vejja]

@prashantpalikhe thanks this is insightful
I am not sure whether the problem is that Cheerio itself is too big to fit in Workers, or whether it takes too much time to process the HTML. What's your opinion there ?
I'm asking because @unr said that he was exceeding CPU limits

[vejja]

@pi0 @huang-julien
We are using cheerio to parse the HTML in our Nitro plugins, but this seems to crash workers
Do you have a recommendation for a faster/lighter library ?
Or alternatively is it possible to hook deeper into Nuxt to avoid parsing HTML strings ourselves and directly use some kind of structured data before it is combined into HTML ?

[prashantpalikhe]

@vejja

but this seems to crash workers

Is this reproducible easily?

Maybe parse5 can be used? Cheerio seems to use it internally. But, if the crashing of workers/exceeding of CPU limit, is easily reproducible, we can try using parse5 directly and see if that behaves better.

[prashantpalikhe]

I think we can better just use regex?

 for (const section of sections) {
    html[section] = html[section].map((htmlString) => {
        return htmlString.replace(/<(script|link)(.*?)>/g, `<$1 nonce="${nonce}" $2>`);
    });
}

Seems to be significantly faster. Any risk you guys see in doing this? @vejja @Baroshem

[vejja]

We used to be doing something similar, but our code quality tool was complaining about this approach. There are so many potential traps, exceptions and special cases in trying to manually parse HTML that apparently it is prone to errors.
@Baroshem shall we reconsider ?

[Baroshem]

Hey guys,

Yes we had a regular regex before. I dont actually remember what was the issue back then. But seeing that this solution could improve the performance I am up for trying it instead of cheerio.

I think we could refactor the current html parsing to the previous approach for 1.1.0 and compare the performance with 1.0.0 (that uses cheerio)

If it will be causing any issues, we can release a patch 1.1.1 with a revert to use cheerio once again.

What do you think?

[prashantpalikhe]

That sounds good.

And what do you guys think about allowing configuration of the sections to loop through.

Use case: if as a developer, I am confident that the body section won't contain any link/style/script tags, then parsing that string (usually the largest chunk) is wasteful.

So, if we could configure that somehow, that would be pretty rad.

[prashantpalikhe]

Also, before we make the regex changes, it would be great to profile the CPU/memory consumption of the regex based apprach and compare with Cheerio.

Any ideas on how to best do that check?

I am using node --inspect right now and doing some memory profiling. But I always find it tedious to parse the result of that.

[vejja]

Hey guys,

Yes we had a regular regex before. I dont actually remember what was the issue back then. But seeing that this solution could improve the performance I am up for trying it instead of cheerio.

I think we could refactor the current html parsing to the previous approach for 1.1.0 and compare the performance with 1.0.0 (that uses cheerio)

If it will be causing any issues, we can release a patch 1.1.1 with a revert to use cheerio once again.

What do you think?

I think the classical example where this fails is if you write a documentation section for how to use some feature that includes the regex, e.g.

Documentation:
To do something with Vue 3, copy the following code
<script setup lang="ts">
...
</script>

Having a DOM Parser allows to make the difference between the tag itself and its innerHTML.

In addition the regex must be able to process both external scripts (<script ... />) and inline scripts (<script>...</script>). Our regex used to do that but it is tricky.

[vejja]

Also, before we make the regex changes, it would be great to profile the CPU/memory consumption of the regex based apprach and compare with Cheerio.

Any ideas on how to best do that check?

I am using node --inspect right now and doing some memory profiling. But I always find it tedious to parse the result of that.

I am using process.memoryUsage() in the code right now, but not sure it is any better as I find it extremely difficult to read too. And it's only available in Node.

I'm trying to see if there would be significant improvement by only invoking cheerio.load once and for all, as this seems to be the slowest part of the code.

[huang-julien]

Hey 👋 just catched up with the discussion. In Nuxt core, we're not currently using any external packages to replace html. We're mainly using regex .

[vejja]

I'm proposing a temporary patch for affected users while we refactor to regexes

[Baroshem]

Hey guys, I have merged the patch to the 1.1.0 branch.

You can either check it on the branch if the application is more performant or wait until I will release the 1.1.0 release this week and we could test it there.

Let me know :)

[vejja]

Hi @Baroshem, I'm proposing an additional patch above, maybe our CF users want to test that

[Baroshem]

Awesome @vejja

Lets merge it to the 1.1. branch and test it there :)

[Baroshem]

@unr @prashantpalikhe @vejja @huang-julien

1.1.0 version has been released with the performance optimizations by @vejja.

Could you check if now it is better? :)

[Baroshem]

@unr @prashantpalikhe any news from your side? :)

[prashantpalikhe]

Hey there, @Baroshem

Sorry, due to some personal issues, I have not been able to code much the last month and a half.

I will test it out soon. But, regardless, on our end, we went with our custom CSP/nonce implementation because we want to skip parsing of the entire body which is significantly large in our case. And, has no scripts/styles/links.

I will check against this new implementation regardless.

[Baroshem]

Thank you @prashantpalikhe and hope everything is ok!

In your custom solution, if you think that there is something that could be done better in Nuxt Security, please do not hesitate to let us know :)