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

refactor: move media type logic to Plan module #2934

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

steve-chavez
Copy link
Member

Part of #2825.

@steve-chavez
Copy link
Member Author

According to the loadtest results we lose some throughput with this change https://github.com/PostgREST/postgrest/pull/2934/checks?check_run_id=16660131983

param head main
throughput 307.90351639998954 315.3078228340249

Which is weird because it's the same logic, just on a different module. I've seen this case before, will check what's wrong with the Plan module.

@wolfgangwalther
Copy link
Member

Remember, a single CI run can always be just random noise. For example you could have had more load on the machine that the job is running on during the "main" part of the test, but not during "head".

Always repeat those tests locally, while keeping load constant. I just did and got:

  • main: 449.36
  • your branch: 449.20

I don't think there is anything wrong here.

@steve-chavez
Copy link
Member Author

Thanks a lot for the help Wolfgang,

Always repeat those tests locally, while keeping load constant.

Yeah, actually I did that and I always got worse results for this PR. Now, after #2940, I don't get bad results, in fact they seem slightly better:

  • main: 565.19
  • this branch: 574.37

Maybe there was some perf improvement on #2940 after all. Anyway, this is looking good to merge now. Postponing this change will make #2825 harder otherwise.

@steve-chavez steve-chavez merged commit 8d3c9f8 into PostgREST:main Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants