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

Refector openswoole #7937

Merged
merged 1 commit into from
Mar 14, 2023
Merged

Conversation

doubaokun
Copy link
Contributor

@doubaokun doubaokun commented Feb 20, 2023

Fix #7928

@doubaokun doubaokun force-pushed the fix_openswoole branch 2 times, most recently from 2d386fc to e42348d Compare February 21, 2023 02:07
"notes": "",
"versus": "php"
},
"postgres": {
Copy link
Contributor

@joanhey joanhey Feb 21, 2023

Choose a reason for hiding this comment

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

Postgres variant is deleted in config.json.
But you created an Open Swoole Postgres server, and never will be executed.

image

Please add again the postgres variant here.

@joanhey
Copy link
Contributor

joanhey commented Feb 21, 2023

Add to #7703
PHP 8.2


RUN apt-get update && apt-get install -y git > /dev/null

RUN docker-php-ext-install opcache pdo_mysql > /dev/null

RUN pecl install openswoole > /dev/null && \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know PHP, but does this pin the version of Openswoole? I noticed that the Dockerfile for the PostgreSQL permutation was using a different approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the Postgres driver is not installed by default with Swoole and OpenSwoole, and need to compile with /configure --with-postgres

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, but the main point still stands - does this pin the version of Openswoole?

Copy link
Contributor

@joanhey joanhey Feb 21, 2023

Choose a reason for hiding this comment

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

No, pecl don't use semver or an updating system.
Both Swoole and OpenSwoole, release a new version every 2-3 months from 2021.
So they use a 'fail fast' solution.

I try that all the PHP ecosystem in this bench, is updated and working.
But I was busy, the last weeks. Both are failing for the last ~3 runs only.
And a lot of times, the run fail for a fw, is downloading from the servers. Not that the fw is failing, so I wait some fails.

Also with PHP, we use the major version. All the minor versions are automatic.
The same with composer, that we use semantic versioning, and still sometimes fail.

When fail I try to fix it or create an issue in the fw (so the bench also is used to find bugs) and if necessary I marked them as broken.
Without the fail fast way, we'll have very old versions. And use a lot of my free time, now I have near to 500 PRs merged. If a need to update the pecl versions each 2-3 months, I will need 4000 PRs to update the ~60 php fws and permutations.

No PHP framework stay more than ~3 months failing in the runs. Swoole was updated Dec, 2022 #7788 and OpenSwoole in Nov, 2022 #7665 and both working.

Only check the work for major PHP versions from 2021: #6184, #6894, #7703 (still updating)

Copy link
Contributor

@joanhey joanhey Feb 21, 2023

Choose a reason for hiding this comment

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

You made the PR to mark as broken a lot of frameworks.
I do it normally for the PHP frameworks. But not when fail for the last 2 runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately it is up to the TechEmpower team to set policies, but my impression is that they prefer when all dependency versions are pinned, which also makes it possible to reproduce results. The reason you have opened so many pull requests is because you have changed one or a few implementations in each one, but there is no requirement to do it that way - if you update versions every quarter, you can just do 4 PRs in a year.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking so, the first time that I tried to update a major PHP version.
Some years ago, with few php frameworks, I created 1 PR to update all the PHP frameworks.
It was a nightmare, in almost all aspects.

Copy link
Member

Choose a reason for hiding this comment

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

It would be ideal if we could pin the version of OpenSwoole. If you used the same opening lines as the postgres dockerfile at least the compilation step could be cached. Would there be any issues using OpenSwoole with other tests if it's using the configure with postgres option?

Copy link
Contributor

@joanhey joanhey Mar 7, 2023

Choose a reason for hiding this comment

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

The mysql variants aren't compiled.
The postgres variant is pined and compiled, but it don't execute till update the bench config. I send a PR to doubaokun:fix_openswoole branch, but still not merged.

The good think is that now the 2 mysql variants dockerfiles are identical, and so 1 variant use 1s.

Copy link
Contributor


RUN apt-get update && apt-get install -y git > /dev/null

RUN docker-php-ext-install opcache pdo_mysql > /dev/null

RUN pecl install openswoole > /dev/null && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@joanhey
Copy link
Contributor

joanhey commented Mar 13, 2023

Ready to be merged.

Later I'll add the Postgre change.

@NateBrady23 NateBrady23 merged commit 34b05d2 into TechEmpower:master Mar 14, 2023
@joanhey
Copy link
Contributor

joanhey commented Mar 22, 2023

@doubaokun after this PR, OpenSwoole have a very big drop in performance.

@doubaokun
Copy link
Contributor Author

@joanhey thanks, will check. The threads reduced to half in this PR, so metrics could be lower and not comparable.

franz1981 pushed a commit to franz1981/FrameworkBenchmarks that referenced this pull request Jun 23, 2023
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