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

Improve Composer Performance #3121

Closed
wants to merge 412 commits into
base: 9.2.x
from

Conversation

Projects
None yet
@lcatlett
Copy link
Collaborator

lcatlett commented Sep 22, 2018

Changes proposed:

  • Add zaporylie/composer-drupal-optimizations package to reduce performance overhead of symfony updates due to composer iterating over all tags. The benchmarks below show a 10 second improvement for composer update and significantly lowered memory consumption.

Benchmarks

Below are benchmarks on a vanilla BLT project

~/sites/bltacsf/blted8 master*
❯ composer update nothing --profile
[379.6MB/16.45s] Memory usage: 379.63MB (peak: 1266.55MB), time: 16.45s

~/sites/bltacsf/blted8 master* 18s
❯ composer require zaporylie/composer-drupal-optimizations
Writing lock file
Generating autoload files

~/sites/bltacsf/blted8 master* 20s
❯ composer update nothing --profile
[282.5MB/6.69s] Memory usage: 282.46MB (peak: 386.61MB), time: 6.69s

anavarre and others added some commits Feb 23, 2018

Fixes #2580: Recent Project Update throws intimidating error. (#2581)
* Fixes #2580: Recent Project Update throws intimidating error.

* Update Plugin.php

Fix typo.
Connects to #2582: Disabling BLT commit message. (#2585)
* Connects to #2582: Disabling BLT commit message.

* Update GitCommand.php
Use HEAD of main Lightning branch (#2592)
* Use HEAD of main Lightning branch

* Trigger rebuild - packagist new deps

* Trigger rebuild - packagist new deps

* Trigger rebuild - packagist new deps

* Trigger rebuild - packagist new deps

* Require HEAD of deprecation-detector until they tag a release that allows for more versions of docblock
Fixes #2566: Allow site to be installed from existing config. (#2590)
* Fixes #2566: Allow site to be installed from existing config.

* Update ConfigCommand.php

* Don't change profiles.
Drush 9 blt doctor fixes (#2598)
* Fix errors in blt doctor.

* Set isDrupalVmLocallyInitialized to true if command is running in the VM.

* Update message with drush site:alias command.
Removing Drupal VM command proxy. (#2597)
* Removing Drupal VM command proxy.

* Fix test.
Fixes from pre-release testing. (#2602)
* Fixes from pre-release testing.

* Add chrome to Drupal VM.

* Warn user forcefully when executing commands outside VM.

* Do not include local.setting.php in CI envs.

* Copy local.settings.php to includes.settings.php.

* Try moving again.
Prevent warnings in update hook. (#2604)
* Do not include local.setting.php in CI envs.

* Copy local.settings.php to includes.settings.php.

* Try moving again.

* Prevent warnings in update hook.
Use --environment option rather than --define. (#2607)
* Use --environment option rather than --define.

* Trim = from beginning of --define value for env.
Require PHP 7.1 in all the right places. (#2611)
* Require PHP 7.1.

* Add sudo to template .travis.yml to prevent timeout.
@aweingarten

This comment has been minimized.

Copy link
Contributor

aweingarten commented Oct 9, 2018

@danepowell, what is the CPU usage. I wonder if the composer CPU goes down enough to allow for any parallelization of composer + node?

@lcatlett, I wonder if it makes sense to fork this and add lightning and core?
https://github.com/zaporylie/composer-drupal-optimizations/blob/master/src/Cache.php#L14

@danepowell

This comment has been minimized.

Copy link
Collaborator

danepowell commented Oct 9, 2018

@aweingarten I checked, CPU usage is basically zero regardless of whether this package is used or not. That jives with my previous experience with composer: the performance bottleneck is disk and network i/o latency, CPU has little or nothing to do with it.

danepowell and others added some commits Oct 9, 2018

Travis shouldn't duplicate Drupal 8.6 tests (#3152)
* Travis should test Drupal 8.7.

* Disable 8.7 tests.
Prevent cache collisions in multisite db-update tasks (#3166)
* Add drush cache dir utility script.

* Enhanced Factory Hook to isolate db-update tasks to tempoary drush cache.

* Update Factory Hooks from BLT template.

* Remove redundant cache clear and rebuild to preserve drush context of current request.

* Style tweaks and setting update version number to 9.2.
@lcatlett

This comment has been minimized.

Copy link
Collaborator

lcatlett commented Oct 16, 2018

benchmarks for oob blt 10.0.x are below. 10 seconds off a 19 second build is rather substantial and this is the only way I've been able to keep Pipelines builds from timing out.

~/sites/blt/blted8 master*
❯ composer update nothing --profile
[388.6MB/19.47s] Memory usage: 388.56MB (peak: 1298.94MB), time: 19.47s

~/sites/blt/blted8 master*
❯ composer require zaporylie/composer-drupal-optimizations
Gathering patches for dependencies. This might take a minute.
  - Installing zaporylie/composer-drupal-optimizations (1.0.2): Loading from cache
Writing lock file
Generating autoload files

~/sites/blt/blted8 master* 22s
❯ composer update nothing --profile
[291.8MB/9.33s] Memory usage: 291.84MB (peak: 401.27MB), time: 9.33s

As far as downsides, this approach will not work for older versions of core or other dependencies that require legacy versions of symfony.

lcatlett and others added some commits Oct 16, 2018

Prevent cache collisions in multisite install tasks (#3171)
* Refactor post-install hook to use temp cache dir per site, use requests instead of ENV.

* Refresh Factory Hooks from template for 9002001.

* Update version number for hook.

* Remove redundant factory hook update.

* Removed unused use statements.

* Style and phpcs updates.
Update BLT settings for site and http/proxy detection (#3172)
* Fix auto detection of sites dir due to core changes.

* Update proxy and host detection for core updates and Acquia VCL changes.

* PHPCS fixes.
Update deploy.md with correct Cloud Hooks links. (#3188)
The Acquia docs link pointed to a stale page and the BLT one pointed to the 8.x branch, both throwing 404's. I've updated both to point to the latest iterations of each page.
Update to Drupal Coder 8.3.x. (#3132)
Fixes #2289 
--------

Changes proposed:
---------
(What are you proposing we change?)

- Update composer contraint on Drupal Coder newest release (8.3.1 as of this date)
- Update composer constraint on squizlabs/php_codesniffer to 3.0.1 (matching Coder)
- Remove the use of a bootstrap file with PHPCS. File paths that are not allowed by filesets won't be sniffed by default.
- Add a tests:phpcs:sniff:modified command to sniff unstaged modified or added files in repo for convenience.
[9.2.x] Fix undefined trusted_reverse_proxy_ips #3214 (#3215)
Fixes #3214 
--------

Changes proposed:
---------
(What are you proposing we change?)

- check if trusted_reverse_proxy_ips is defined as an array
- if not, set an empty array

Steps to replicate the issue:
----------
(If this PR is not fixing a defect/bug, you can remove this section.)

1. blt setup
2. bootstrap drupal

Steps to verify the solution:
-----------
(How does someone verify that this does what you think does?)

1. apply the patch
2. blt setup / bootstrap Drupal
3. check to ensure following warning does not appear:

```
Warning: in_array() expects parameter 2 to be array, null given in require() (line 27 of /app/vendor/acquia/blt/settings/blt.settings.php).
require('/app/vendor/acquia/blt/settings/blt.settings.php') (Line: 806)
require('/app/docroot/sites/naswa_main/settings.php') (Line: 125)
Drupal\Core\Site\Settings::initialize('/app/docroot', 'sites/naswa_main', Object) (Line: 1056)
Drupal\Core\DrupalKernel->initializeSettings(Object) (Line: 655)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
```

@lcatlett lcatlett force-pushed the lcatlett:perf-composer branch from 651e8f0 to f4ba703 Nov 10, 2018

@lcatlett lcatlett closed this Nov 10, 2018

@lcatlett lcatlett reopened this Dec 9, 2018

@lcatlett lcatlett closed this Dec 9, 2018

@lcatlett

This comment has been minimized.

Copy link
Collaborator

lcatlett commented Dec 9, 2018

Coming back to this, it looks like a few projects are using zaporylie/composer-drupal-optimizations and seeing dramatic performance improvements. I will open a new PR with updated benchmarks since this one is so out of date. Symfony flex has a similar approach but we arent really leveraging any recipes so it seems like unneeded overhead

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented Dec 9, 2018

@lcatlett - Awesome, thanks! I was just testing this on some non-BLT projects and found that it increased performance for composer require and reduced memory usage, both by half or more! And with PHP 5.6 (for the brave few still running it), it's even more dramatic.

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented Dec 31, 2018

To put a pin in this issue... it looks like @grasmash filed a new PR with just the addition of this library 15 days ago: #3287

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented Dec 31, 2018

(However, that's only in 10.x)

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