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

ci: add loadtest workflow & script #6129

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tchapacan
Copy link

@tchapacan tchapacan commented Nov 3, 2024

Purpose of this PR is to add a new github workflow and script which will run the benchmarks for express.

  • Automatically at each PR opened & synced
    • Will compare master branch and the branch from the PR

    The result will be added as comment in the PR.

  • Manually, you can dispatch the workflow
    • You can compare 2 versions of express being published
    • You can compare 2 branches of the express repo
    • You can compare one branch of the express repo and one version of express being published

    The result will be added as summary of the workflow run

The new script is based from your initial work here https://github.com/UlisesGascon/express-performance-comparator

I reused the benchmarks/middleware.js with minimal changes to not alter the original work.

I have created a test repo if you would like to test the workflow here => https://github.com/tchapacan/express-test-wf/

This PR seems a little big to review at once and lack of tests and docs (if necessary), I can split it in smaller PRs if you prefer, ex:

  • One PR to add the load-test-workflow.js script
  • One PR for tests if relevant? (don't really know where to land them smoothly in the repo)
  • One PR to modify the benchmarks/middleware.js script
  • One PR to add the load-test.yml workflow
  • One PR to document the workflow usage somewhere relevant (don't really know where to land it smoothly in the repo)
  • One PR to modify the number of middleware/connection to fine tune benchmark scenario afterward?

LMK if this proposition suits you. I understand that CI might need further discussion between you, specifically if you have already planned/discussed something. Maybe this deserve it's own repo (if more feature, docs, tests are needed then) ?
Furthermore we have to keep in mind that benchmark will run on the runner and the result will strictly depend on it.

Anyway, I'm 100% open to discussion/changes about that, hope it's helping you a little, cheers!

@tchapacan tchapacan changed the title feat: add loadtest workflow & script ci: add loadtest workflow & script Nov 4, 2024
@UlisesGascon
Copy link
Member

Thanks a lot @tchapacan! This is a huge step for the project ❤️

I will love to have a deep discussion in this PR with the community too and later on we can split this PR on smaller pieces once we are all in the same page.

cc: @expressjs/express-tc @expressjs/triagers @andrehrferreira @cesco69 @nigrosimone @faulpeltz

Note: Just to avoid accidental merges I will change this PR to a draft

@UlisesGascon UlisesGascon self-assigned this Nov 11, 2024
@UlisesGascon UlisesGascon marked this pull request as draft November 11, 2024 16:14
@tchapacan
Copy link
Author

No problem my pleasure, happy it help and could give a starting point even if it's basic 😄. Then I'll let you discuss with the community about how you wanna align and plan that (I'm sure there might be other use case that could be turn into reusable workflow, even in a dedicated repo) , I'll stick around this PR 👀

@wesleytodd
Copy link
Member

Yeah, one thing is I dont want to rely on GH runner perf for our benchmarks. They are VERY unreliable and we don't control the execution environment enough for them to be reliable. Unfortunately I am just catching up on backed up notification queue, and so I will not be able to help with more direction on this for a bit. Could we keep this in draft until we can schedule a deep dive conversation on one of our working sessions and invite you @tchapacan?

@tchapacan
Copy link
Author

Yes ofc I agree and well aware of that, that's why I have briefly precised it in my initial message. A possible solution for that would be a dedicated runner, but this naturally bring the fair question of costs and maintenance. As I lack history and context, I might not be the best person to push in a specific direction.
No problem ofc take your time and keep this PR warm as a draft for later! I would be happy to attend one of your working sessions when this topic will be on the table. It will give me further input to help me plan the next contributions for this subject (depending on the schedule and availability of everybody)

@andrehrferreira
Copy link

Hey guys, I'm going to take some time to take a look and come back with a summary. Based on the tests I've done so far, the biggest performance problem that Express faces is the use of Object.defineProperty, as described in my issues. I believe that to considerably improve the project's performance, this point needs to be resolved as soon as possible.

#5998

@wesleytodd
Copy link
Member

Yeah, there are tons of low hanging fruit on perf improvements. I would like to take a two prong approch to this:

  1. setting up infra to allow for reliable and meaningful monitoring
  2. landing incremental improvements prioritizing simplicity first and then slowly tightening the perf budget

The current problem in #5998 is that we are spread too thin and folks are working in too many directions at once. IMO we should pop the stack first and discuss meta questions before we get into the weeds, and that has not happened yet. If someone here would like to help drive that, I think the best way is to open an RFC to start a performance working group where we can work together on the plan.

@andrehrferreira
Copy link

@wesleytodd I think the biggest challenge is the following: there are several parallel improvement processes, but to correct the biggest problem requires a major change that may take a long time to implement, and will clash with other changes. I believe organizing this flow is more complicated than making the changes.

@wesleytodd
Copy link
Member

I am not sure I follow your comment. It seems like you are saying "this is big and will take a long time" but also saying "organizing the work is more complicated than the work itself"? Can you help me understand in more concrete terms what you are suggesting?

@andrehrferreira
Copy link

@wesleytodd Sorry, English is not my native language, I said that the change is simple, it will just take a little work to test everything and pass the tests, which in my opinion will be more complicated to organize with other PRs that are being considered at the moment.

@wesleytodd
Copy link
Member

No worries on the language barrier. Feel free so post also in your native language and I can work to translate on my end as well (maybe post both an english and portuguese version of the same thing?).

I don't think this is the place to discuss if it is worth organizing on the larger scale. One way or another we have to do that, and I am perfectly fine driving that myself. I have just seen too many folks produce a lot of long term complexity in the name of perf based on incomplete picture of perf, and I will not let this project fixate on micro-benchmarks and micro-optimizations if I can help it. Feel free to continue to propose improvements, we will likely discuss and assess them as part of the proposed WG (which you are welcome to participate in or ignore).

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

Successfully merging this pull request may close these issues.

4 participants