Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved benchmarking of uWebSockets.js #6318

Closed
wants to merge 4 commits into from

Conversation

mindplay-dk
Copy link

  • Add a dedicated benchmark for uWebSockets.js, so we can compare against frameworks that use it.
  • Fix a performance issue with existing routejs-uwebsocket benchmark.
  • Upgrade to latest version of uWebSockets.js in relevant benchmarks, for realistic comparison among them.

@mindplay-dk mindplay-dk requested a review from waghanza as a code owner April 28, 2023 10:11
Copy link
Collaborator

@waghanza waghanza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making some update, but unfortunately I'm not willing to accept uWebSockets.js here, because the author do not want to #1064

@mindplay-dk
Copy link
Author

That was 2 years ago. Is the benchmark still unreliable?

Has the author suggested anything to improve it? Those comments are incredibly hostile and unhelpful. I don't even understand exactly what his complaint is. Something about memory not being measured correctly? I imagine most people would be more interested CPU usage. RAM is cheap. If the memory benchmark is inaccurate, I would suggest disabling it until we can figure it out.

And you already have uWebSockets being benchmarked in there. (Twice.)

How is a project supposed to make progress if we don't accept PRs that improve things? Even if there are still problems, this is at least a few steps in the right direction.

@waghanza
Copy link
Collaborator

I understand you frustration @mindplay-dk.

You are right when you are saying that uWebSocket is tested here (in fact more that twice), but this is behind the scene, to have a reference implementation, we must have @uNetworkingAB non opposition.

I can also understand that this project is young, and need improvements, but not sure using code with explicit opposition of core contributors will be for the best of this project.


You have updated uws for siffr and routejs, maybe you can split this PR to have this part approved

@mindplay-dk
Copy link
Author

This PR only exists because I wanted to know how much overhead routejs (et al) add to uWS base overhead.

Is there anything I can do to help improve the situation to please that maintainer?

Anything with uWS in it already the fastest thing on that list, so I don't know what he's so unhappy about. 🤷‍♂️

@mindplay-dk
Copy link
Author

Would this help?

@waghanza
Copy link
Collaborator

I think so @mindplay-dk. I am actually working on #3958, so I can afford time for this.


I suggest to :

  • remove raw uws from here, so I can approve
  • work on what you propose, either you or me, but it requires a long run (and I can not do it rn), I mean, it requires to change one implementation, as for review ... and change the ~250 here
  • re-suggest to its author to submit uws after changing the base implementation

@kartikk221
Copy link
Contributor

@mindplay-dk The sifrr benchmark pretty much represents uWebsockets.js performance because it directly exports uWebsockets.js and provides a functional approach for doing things like writing headers which is pretty much zero cost.

@mindplay-dk
Copy link
Author

@mindplay-dk The sifrr benchmark pretty much represents uWebsockets.js performance because it directly exports uWebsockets.js and provides a functional approach for doing things like writing headers which is pretty much zero cost.

That was my point - since we're already benchmarking uWebSockets.js indirectly, it would be nice to have the direct benchmark for comparison, precisely so the siffr benchmark can be seen in perspective to the "base load". :-)

@waghanza
Copy link
Collaborator

waghanza commented Jun 2, 2023

Agreed with you @mindplay-dk, but using a piece of code (even OSS) with opposition of its author, is not ... what I expect to do.

Can you update you PR and remove direct usage of uws ? and probably open an other PR for that and ping its author

@kartikk221
Copy link
Contributor

kartikk221 commented Jun 2, 2023

@mindplay-dk It is certainly a bit misleading to have sifrr and routejs-websocket up there because their slight performance comes from not caching request metadata like headers, query etc etc. If there was an async handler scenario or something similar of that sort, you would see a performance decrease. This is why while hyper-express is not the top in the Javascript ecosystem, it is the top in the ability to provide an Express-like experience with no questions asked or catches to the request life cycle. You don't have to worry about reading headers before doing any async operation because hyper-express caches the metadata for you from uws even if it costs performance.

The only solution to this problem is to add more tests which access various request metadata and also test async vs. sync handling along with body parsing to really test all of the webserver's features but that is of course easier said than done given how many webservers this project supports.

@waghanza
Copy link
Collaborator

waghanza commented Jun 3, 2023

@kartikk221 I agree with you when you are saying that we need a more complex scenario (in order to have like a standard workload test), but if some framework want to cache it their choice, a design choice that should not be used to rank some frameworks

@kartikk221
Copy link
Contributor

@kartikk221 I agree with you when you are saying that we need a more complex scenario (in order to have like a standard workload test), but if some framework want to cache it their choice, a design choice that should not be used to rank some frameworks

I see what you are saying and would agree to an extent but it is also important to consider that the caching being done in this case from the uWS request metadata is pretty much necessary If you are to do any asynchronous work in your routes which almost all Node.js developers will be doing. Essentially, uWS requires you to read any metadata about the request in the initial synchronous call from its route handler. This means If you don't know what you want ahead of time, you won't be able to read metadata. This can start to become a big problem when you begin working with middle-wares and async handlers and you will be accessing various metadata like headers, query, path etc etc throughout your code.

If the performance of some web servers drops the moment you test them with something like reading headers or doing any sort of async work, which is something almost 100% of Node.js developers will be doing, then did those webservers truly represent their performance against others well? That's the question we want to consider.

@waghanza
Copy link
Collaborator

waghanza commented Jun 3, 2023

I think we are on the same line @kartikk221. Some frameworks caches, some frameworks don't. It their choice. To have the fairest benchmark we can, we should stick to have some realistic scenarios (async part of them imho), so if some frameworks are not able to have good figures for async, not sure we should worry here ... but again having standard workload scenarios is the biggest discussion we need to have before considering this project as mature enough

@kartikk221
Copy link
Contributor

Agreed, adding more workloads like async, echoing body, headers etc etc shouldn't be unfair but rather improve the accuracy of the results.

@waghanza
Copy link
Collaborator

waghanza commented Jun 3, 2023

Yes, I'm ok to start a discussion based on this. There is a lot of ideas here, but separated in may issues

@cyrusmsk
Copy link
Contributor

cyrusmsk commented Aug 8, 2023

Yes, I'm ok to start a discussion based on this. There is a lot of ideas here, but separated in may issues

WebSockets already merged, but could you elaborate about plans for this project? About new improvements for the test?

@waghanza
Copy link
Collaborator

waghanza commented Aug 8, 2023

Maybe a more complete scenario which mimic database calls for example, some random number generation .... @cyrusmsk

@waghanza waghanza closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants