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

DX: use only PHP modules that are required #6954

Merged
merged 13 commits into from
May 19, 2023
Merged

DX: use only PHP modules that are required #6954

merged 13 commits into from
May 19, 2023

Conversation

kubawerlos
Copy link
Contributor

@kubawerlos kubawerlos commented May 14, 2023

Goal: performance? Smaller resource usage?

Initial situation (see logs from 1st commit):

php -m | wc -l && php -m
88
[PHP Modules]
amqp
apcu
ast
bcmath
bz2
calendar
Core
ctype
curl
date
dba
dom
ds
enchant
exif
FFI
fileinfo
filter
ftp
gd
gettext
gmp
hash
iconv
igbinary
imagick
imap
intl
json
ldap
libxml
mbstring
memcache
memcached
mongodb
msgpack
mysqli
mysqlnd
odbc
openssl
pcntl
pcre
PDO
pdo_dblib
PDO_Firebird
pdo_mysql
PDO_ODBC
pdo_pgsql
pdo_sqlite
pdo_sqlsrv
pgsql
Phar
posix
pspell
random
readline
redis
Reflection
session
shmop
SimpleXML
snmp
soap
sockets
sodium
SPL
sqlite3
sqlsrv
standard
sysvmsg
sysvsem
sysvshm
tidy
tokenizer
xml
xmlreader
xmlwriter
xsl
yaml
Zend OPcache
zip
zlib
zmq

[Zend Modules]
Zend OPcache

After changes in this PR (see commit "Add zip"):

php -m | wc -l && php -m
29
[PHP Modules]
Core
curl
date
dom
filter
hash
json
libxml
mbstring
openssl
pcntl
pcre
Phar
random
Reflection
session
SimpleXML
sodium
SPL
standard
tokenizer
xml
xmlwriter
zip
zlib

[Zend Modules]

@kubawerlos kubawerlos marked this pull request as ready for review May 14, 2023 19:34
Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

I don't know whether we should merge it. Comparing CI workflows, the "Setup PHP" step takes similar time before and after change, so there's no gain in terms of running jobs.

The good part is that we have explicit list of extensions and we don't bloat runtime with them. If there is any code that depends on the list of installed extensions, it now has less to process. But it is micro-optimisation 😅.

Im am not against it, but at the same time I am not in favor that much. I am going to give approval and you can decide @kubawerlos.

Or maybe @keradus can look at it too.

.github/workflows/ci.yml Show resolved Hide resolved
@kubawerlos
Copy link
Contributor Author

Comparing CI workflows, the "Setup PHP" step takes similar time before and after change, so there's no gain in terms of running jobs.

@Wirone TBH I didn't even look at the times of separate steps, I've compared times of jobs - this PR:
image

with times of this PR:
image

@Wirone
Copy link
Member

Wirone commented May 18, 2023

Those times are random and undeterministic 😅. Some are better, some are worse 🤷‍♂️. It's OK for me both ways, I don't have much experience with that action so I can't tell which approach is better. I can ask somewhere 😉.

@kubawerlos
Copy link
Contributor Author

They are not that random, job "CI / PHP 8.2 with calculating code coverage (pull_request)" (usually the slowest one on linux) is 23 minutes here, 18 here and 16 here - 3 recent PRs with passing build - so nowhere near 11 minutes as in this PR.

@Wirone Wirone merged commit 4afe389 into PHP-CS-Fixer:master May 19, 2023
@Wirone
Copy link
Member

Wirone commented May 19, 2023

Thank you @kubawerlos 🍻

@Wirone Wirone added the topic/ci Github Actions, tooling label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement topic/ci Github Actions, tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants