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

Update the set of broken implementations #7928

Closed
wants to merge 5 commits into from

Conversation

volyrique
Copy link
Contributor

@volyrique volyrique commented Feb 17, 2023

This pull request updates the set of broken implementations based on the following runs:
adce24e2-9277-45b2-845c-3dbce439d727
05ddf07c-bbc9-4cea-8799-c1103640136c
3d6c6cc0-92ff-4861-86dc-42cef0ecc50f
Note that the code in the repository has not been deleted - it is just not going to run in CI and continuous benchmarking.

As a small courtesy, I am pinging the contributors to the affected implementations, at least in the cases where I could determine a relevant person - apologies to those who have been mentioned by mistake:
@5hirish
@achlipala
@agrison
@bdw429s
@cacoco
@caturner81
@chunariov
@daviddenton
@doubaokun
@dragosv
@fakeshadow
@gi0baro
@gkbrk
@gotzmann
@greenlaw110
@hamiltont
@itsumura-h
@jcheron
@joanhey
@Kaliumhexacyanoferrat
@kekekeks
@kubo39
@lucipacurar
@luoxiaojun1992
@mkurz
@polachok
@richerarc
@seanmonstar
@steveklabnik
@sumeetchhetri
@theLastOfCats
@tiangolo
@tommilligan
@whitfin

There are also plenty of implementations, I think, that consistently have issues with one or a few of the tests, so the question is what we should do about them. The first option is to leave them alone (which is this PR's approach), but we could also mark them as broken - unfortunately that means that perfectly working functionality would be excluded from continuous benchmarking. The middle ground is to remove the affected test entries from the relevant benchmark_config.json and config.toml files.

P.S. FYI here is the list of implementations that have at least one permutation tagged as broken in the past:

C/octane
C/onion
C++/cpoll_cppsp
C++/ffead-cpp
C++/ulib
Erlang/chicagoboss
Go/revel
Groovy/hot
Haskell/servant
Haskell/snap
Haskell/spock
Haskell/yesod
Java/dropwizard
Java/spring-webflux
Lua/octopus
OCaml/morph
Perl/dancer
Perl/kelp
Perl/web-simple
PHP/hamlet
PHP/hhvm
PHP/hyperf
PHP/workerman
Python/webware
Python/weppy
Ruby/rack
Swift/kitura
Swift/perfect

Use the following command to obtain the list:
find -name benchmark_config.json -exec grep -H broken '{}' \; | cut -d: -f1 | sort -u | cut -d/ -f3-4

@steveklabnik
Copy link
Contributor

I'm happy to attempt to fix my implementation but it may take me a few days, and so I don't mind if it's marked as "can't run" or whatever in the meantime.

@volyrique
Copy link
Contributor Author

Sure, there is no rush. As I said, no code is being deleted at the moment, so it won't be necessary to review everything from scratch once a fix becomes available.

@fakeshadow
Copy link
Contributor

fakeshadow commented Feb 17, 2023

for xitca-web-iou I would not consider it broken as the failed bench is from the older linux kernel version of host OS. The bench itself would likely to work as intended after host OS updated to ubuntu 22.04 which is already planed for Round 22 according to #7321

@volyrique
Copy link
Contributor Author

Fair point, but even if the implementation is technically not broken, it is going to waste resources (mainly runtime) in the continuous benchmarking environment. IMHO if the TechEmpower team does not intend to perform an OS upgrade soon, then it still makes sense to disable it.

@fakeshadow
Copy link
Contributor

It's Ok to disable it sure. Though personally I would like to keep it up as a reminder of tfb lagging on the OS part and not supporting io_uring properly. It can help others to know if and when they can introduce modern io stack to their tfb bench.

@volyrique
Copy link
Contributor Author

volyrique commented Feb 17, 2023

You should mention this potential upside to an OS upgrade in #7321 - right now the people there are mostly concerned with the Spectre mitigations and possibly that Ubuntu 18.04 is running out of support soon.

P.S. It looks like @dependabot had broken several flask permutations yesterday, so I tagged them as well; also, made a quick fix to openswoole-postgres.

@bdw429s bdw429s mentioned this pull request Feb 17, 2023
@bdw429s
Copy link
Contributor

bdw429s commented Feb 17, 2023

Please don't disable CFML. I have sent a pull which should contain the fix for it here:

#7929

I don't know how long it's been erroring, but I would have happily updated it right away if I had known. Is there a way to subscribe to notifications when a build fails? If not, that would be a great enhancement!

@volyrique
Copy link
Contributor Author

volyrique commented Feb 17, 2023

No problem, I can remove the CFML implementations from the changes.

I don't know how long it's been erroring...

Since the end of October, at least.

Is there a way to subscribe to notifications when a build fails?

AFAIK no, you just have to keep an eye on the results dashboard.

@tommilligan
Copy link
Contributor

Thanks for the heads up. I think I was pinged for Rust/tide: fyi I'm no longer interested in maintaining this, so unless anybody else wants to pick it up, I'm happy for it to remain disabled.

@joanhey joanhey mentioned this pull request Feb 19, 2023
@joanhey
Copy link
Contributor

joanhey commented Feb 19, 2023

About PHP
PHP 7.4 EOL in November 28, 2022, so any fw using it will fail.

@jcheron
Copy link
Contributor

jcheron commented Feb 19, 2023

  • Ubiquity with Roadrunner is failing a long time ago @jcheron

@Lapinskas is the creator of Roadrunner-ubiquity.
I've requested him before without success. So we can mark these 2 tests as broken.
see Lapinskas/roadrunner-ubiquity#28

@kaznovac
Copy link
Contributor

@joanhey

I've just took a glance arg the log - sorry I can't provide support in foreseeable future.

the composer install fails as symfony/flex locks sf's dependencies to 5.1
This is the offending line


Set it to 6.* - and the install should complete...

The symfony/runtime dependency is missing (it is not required, and has some performance penalty - but it's the sf recommended way); it will ease testing with multiple PHP 'runtimes'

I've opened the issue on symfony's repo (hope someone will answer the call): symfony/symfony#49447

@gi0baro
Copy link
Contributor

gi0baro commented Feb 20, 2023

@volyrique regarding weppy, feel free to completely remove the code from the suite, as the project is no longer maintained

@doubaokun doubaokun mentioned this pull request Feb 21, 2023
@cacoco
Copy link
Contributor

cacoco commented Feb 22, 2023

@volyrique it looks like a simple misconfiguration in settings for Finatra due to some underlying refactorings that happened a bit ago. I can put up a PR to address it. Thanks.

@volyrique
Copy link
Contributor Author

volyrique commented Feb 22, 2023

@nbrady-techempower This PR was closed by mistake, I believe, and I can't reopen it. You can also merge it (as long as it is acceptable, of course) - all the test failures are either the special cases I mentioned in my initial message (and I would appreciate feedback on which of the 3 options I presented would be the best way forward), or I left broken implementations as they were because there were open PRs that fixed them and were passing the tests.

@volyrique
Copy link
Contributor Author

An updated version of these changes has been merged in PR #7969.

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.

10 participants