Benchmarks and Optimizations

[metcoder95]

Hey! 👋

Wanted to open a discussion just to get your thoughts on this as I have been thinking about this topic for a while.

I've seen recently quite a few PRs for doing optimizations over fetch especially, and I believe and appreciate all of them as it's super helpful to have people contributing to those ends. Highly appreciated.

Nevertheless, I'm starting to see a pattern of constant micro-optimizations (sometimes companioned by micro-benchmarks) that focus on pretty specific parts of the whole flow on fetch that often than not might not make a big impact if we take a holistic view over the fetch implementation (e.g. cold-paths, use cases that are not often seen, etc.).

I believe that those contributions can be of even greater value if we shift that focus to the holistic view of fetch and how the numbers change based on the changes proposed and if they are of impact or not, as it will help assess the value of the change as well as what other parts we can shift our focus to make fetch faster (or even undici).

For instance we can focus on having a better benchmark setup for fetch only, its Web APIs (e.g. Cookies, CacheStorage, WebSocket, etc.).

Feel free to discard the idea, or maybe propose something different 🙂

cc: @nodejs/undici

[Uzlopak]

Can we have an issue to discuss this instead of discussions? i think it it is better to discuss this in a linear way instead of the non-linear way of github discussions.

[Ethan-Arrowood]

In my opinion the only requirements for our fetch and other whatwg api implementations is to (a) pass WPT tests and (b) match non-tested browser behavior as closely as possible. Now of course we are a different environment from the browser so spec deviations are okay (we already do this with headers, core, and caching (and probably some other things)).

With that said, it's in my opinion that we can do whatever we want in our implementation. Take whatever shortcuts we want, make optimizations, etc. As long as we achieve the two requirements I just mentioned, we are in business.

@KhafraDev has taken over a majority of the web api maintenance these days, and I know its his opinion that we don't deviate from the spec even in the implementation so that it is easier to maintain over time. I think that is fine, especially since he is the one doing most of the work, but I do believe it will limit our performance optimization capabilities.

I spoke to Luca from Deno about how he has improved the performance of Fetch. Apparently, their implementation is a lot like what I described. They don't strictly follow every step of the spec, but assert that the behavior is as close to that of the browser as possible (and making the necessary server-side changes like we do).

I wish we could contribute changes to the whatwg spec that makes these apis compatible with both environments, but that is out of scope for this discussion.

[KhafraDev]

a majority of the overhead in fetch is from webstreams

[KhafraDev]

I haven't found any benchmarks for Deno's fetch, but I know for a fact there are many parts of their implementation magnitudes slower than us. We also pass more WPTs than they do in many aspects. For example, their Headers implementation was 20x slower than us maybe a year ago, and since then our implementation has only gotten faster (same for Request / Response), theirs has largely stayed the same.

As Matteo, Rafael, and others have pointed out, the main issue in is webstreams, which we can work around. Write a class that extends the node Readable/Transform stream, implement methods similar to webstreams, and replace them internally. A web readable stream is only exposed when accessing response.body so it would only need to convert it then (which node supplies utilities to do).

Following the spec is not what makes fetch slow, it's the reason node is the first platform to implement a number of specs and spec changes (we were the first to ship the duplex: 'half' change, Response.json, first platform to experiment with WebSocketStream, etc.). It's also why a complete outsider to the undici codebase could understand and write the EventSource implementation we now have.

[Ethan-Arrowood]

I think Deno might have us beat on some stream operations because they deviate from the spec steps. At least that was word-of-mouth, I haven't seen benchmarks yet.

[metcoder95]

I'm not as expert as you guys in the topic, and I totally respect the direction you'd like to follow towards fetch (specially around following the spec or not), and help you in the much extent I can!

I'd would like to see if we can carry some focus to start having a baseline around fetch and its Web API performance, and assess new additions towards that baseline so we can have a better idea of what are really improving here (though we already know that the Web Streams are the real pain).

We can start with baby steps, the current benchmarks goes directly against undici and fetch combined, but I believe that fetch can have its own section where we start adding benchmarks around the most common operations of fetch (similar to what we do for undici, or at least partially do).

e.g. fetch doing POST, fetch with simple GET, etc.

[Uzlopak]

I personally prefer the microbenchmarks being with the PRs. @tsctx was before just posting them into the initial PR comment. But we should keep them with the repo. Usually I checkout the performance pull requests and run the benchmarks and maybe do some code golfing.

Also imho we should optimize everything. even cold paths.

[tsctx]

Should I add all my PR benchmark scripts?

[Uzlopak]

I do so, and my PRs got merged with the benchmarks. I especially restructured the benchmark folder and ordered topic, so that it kind of has a chance to be found when we touch again the same piece of code.

[metcoder95]

I see what you mean, and is totally valid; though we should assess here the balance between the tradeoffs that a proposed optimization sets over the table.

Having as a goal to optimize as much as we can over the overall scope of fetch should be the north-star that keep us away from doing micro-optimizations that can lead to more harder to maintain code that can potentially harm the overall state of the implementation with little or no gain over the common fetch use-case.

[Uzlopak]

To be honest: I thought that #2756 by @tsctx was a total overkill in regard of additional costs of maintainability in comparison of the promised performance gain. But #2809 was fixing a valid finding, but I thought that it was again trying to reinvent the wheel. Instead of actually making the most performant function/operation it was again some reimplementation of the operation we want to replace.
Thats why I proposed #2817 based on #2809 but reduced it to the actual gist. Was kind of sad, when @ronag closed it as non necessary because being hot path.
I dont think that there is a big tradeoff between in #2817 while #2756 was a huuuge tradeoff.
Nothing against @tsctx, i like his/her(?) work alot.

Performance gains are usually possible by looking at the datastructure and ask if it is really necessery to it like that.

E.g. #2903 by @tsctx. It optimizes one edge case. Waaaaait.... While I am writing this, I realize we maybe introduced a bug with that PR. Have to investigate

[Uzlopak]

Ok, there is no bug ;).

So what I wanted to say: e.g. in get [kHeadersSortedMap] it could be faster to not repush everything into the headers array, but to use the sorted array and maybe if cookies is length > 1 to splice set-cookie entries into headers.

[metcoder95]

I wanted to keep open the discussion for a little longer to have more of your thoughts about it, I'll move it to an issue to potentially draft something there 🙂

@tsctx I really liked what you did for the benchmarks over the POST, would you like to help maybe drafting together some baseline benchmarks lonely for fetch and its Web API?

I'll try to draft something later this week, but any suggestion is welcomed

[tsctx]

Yeah, let's discuss it in the PR.