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

feat(adr): establish a performance working group #306

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wesleytodd
Copy link
Member

@wesleytodd wesleytodd commented Nov 14, 2024

This proposal would create a Performance Working Group. See the RFC content for details.

NOTE: this WG would operate under the STF proposal we have made. More details on that to come as we work through the process with the German Govt.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

LGTM

@andrehrferreira
Copy link

Hey guys, how are you?

I took some time off to take a year-end break, I recently performed some tests using my integration tool with the Node.js inspector, I'll leave the link to the test repository, https://github.com/andrehrferreira/express-benchmarks, the test is very simple, I uploaded an instance of express without any middleware and performed the monitoring using the inspector, then I used autocannon to trigger multiple requests in this application using the command autocannon -c 100 -d 40 -p 10 localhost:3000

As a result, my system creates a cpuprofile file that I analyze using the application https://discoveryjs.github.io/cpupro, I'll leave the profile file in the repository for deeper analysis, but basically, 37.5% of the processing time was from internal express functions, with 13.8% from response.js and 9% from the router, which in my opinion are the main ones bottlenecks of the current version, 7.3% in middleware/init.js, looking deeper the most worrying is in fact the send function for response, followed by the header and contentType, as I had already commented before in my point of view the main problem is in the use of Object.defineProperty, however I will continue analyzing other important points to be optimized

@wesleytodd wesleytodd mentioned this pull request Jan 22, 2025
@wesleytodd
Copy link
Member Author

Hey @andrehrferreira, thanks for the post. These kinds of things are great, but for the project the larger problem is two fold:

  1. Tracking performance as we change and improve the libraries (also taking into account runtime improvements)
  2. Testing real application performance where the framework is impactful

It has been my experience that many engineers will spend a lot of time on optimizing some small subsystem that has little to no impact on the real world performance of their applications. My goal with setting up this working group is to level up everyone who wants to do perf work and to give ourselves the tools to do the right analysis for the right type of changes. Please keep finding things, and preferably open PRs with a change, a before and after benchmark result, and a description of how that impacts real applications (is it hot path, startup, etc), but also lets post that in the applicable repos instead of here where we are bootstraping the WG. Thanks!


Onto the topic of the WG, I am going to be just starting the work on this next week. I am going to update this proposal soon to include some more detailed goals/outcomes so we can be clear on what this is for.

@andrehrferreira
Copy link

@wesleytodd, How are you?

When I opened this discussion a few months ago, I was at the beginning of a project I'm working on. Due to the understandable long period that Express requires to upload updates of this size, I ended up rewriting all the functions in parallel in a separate project that I'm already using in the application. I'll leave the repository here https://github.com/cmmvio/cmmv-server

In addition to the Express codes, I used part of Fastify and Koa for this version. What I realized in this process, following what you're saying, is that Express currently has 3 serious problems. The first is related to the way the system packages the req/res using Object.defineProperty. This in itself is practically 90% of the performance problem. The second problem is related to the Router, which is slow compared to other methods. In my project, I'm using https://www.npmjs.com/package/find-my-way

The third, relatively minor problem is the middleware processing lifecycle, which Fastify solved it using Hooks, and that was the path I followed as well. Below I will leave the numbers from the last performance test I did:

Benchmarks

Framework Version Router Requests/s Latency (ms) Throughput/Mb
bare v20.17.0 91084.8 10.51 16.24
fastify 5.2.1 87495.9 10.94 15.69
connect-router 1.3.8 86404.8 11.05 15.41
micro-route 2.5.0 86115.2 11.10 15.36
cmmv 0.8.0 85812.8 11.15 15.39
adonisjs 7.4.0 83334.4 11.52 14.86
koa 2.15.3 78946.0 12.17 14.08
koa-isomorphic-router 1.0.1 71901.6 13.42 12.82
koa-router 13.1.0 70117.2 13.77 12.50
microrouter 3.1.3 63030.8 15.37 11.24
hapi 21.3.12 60592.8 16.00 10.81
restify 11.1.0 58288.0 16.65 10.51
fastify-big-json 5.2.1 21667.6 45.64 249.29
express 5.0.1 21299.2 46.42 3.80
express-with-middlewares 5.0.1 18680.4 53.00 6.95

@wesleytodd
Copy link
Member Author

I just pushed some updates to the proposal. Adding the TC for a review now that it has more details.

@wesleytodd wesleytodd requested a review from a team February 4, 2025 18:51
Co-authored-by: Sebastian Beltran <bjohansebas@gmail.com>
@@ -0,0 +1,71 @@
# ADR [Number]: Performance Working Group
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to add the number before merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants