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

Pre-commit Twig linting is slow, even when no .twig files have been staged for commit #3009

Closed
crittermike opened this issue Aug 14, 2018 · 8 comments
Labels
Support A support request

Comments

@crittermike
Copy link

crittermike commented Aug 14, 2018

My system information:

  • Operating system type: macOS (BLT is running in a Docker container, specifically the Docksal CLI container)
  • Operating system version: 10.13.6
  • BLT version: 9.1.1

Output of blt doctor:

+---------------------------+------------------------------------------------+
| Property                  | Value                                          |
+---------------------------+------------------------------------------------+
| %paths.%root              | /var/www/docroot                               |
| %paths.%site              | sites/default                                  |
| %paths.%modules           | sites/all/modules                              |
| %paths.%themes            | sites/all/themes                               |
| %paths.%config-sync       | /var/www/config/default                        |
| %paths.%files             | sites/default/files                            |
| %paths.%temp              | /tmp                                           |
| %paths.%private           | /var/www/files-private                         |
| admin-theme               | seven                                          |
| alias-searchpaths.0       | /var/www/drush/sites                           |
| blt-version               | 9.1.1                                          |
| bootstrap                 | Successful                                     |
| composer-version          | Composer version 1.6.3 2018-01-31 16:28:17     |
| config-sync               | /var/www/config/default                        |
| db-driver                 | mysql                                          |
| db-hostname               | db                                             |
| db-name                   | default                                        |
| db-password               | user                                           |
| db-port                   | 3306                                           |
| db-status                 | Connected                                      |
| db-username               | user                                           |
| drupal-settings-file      | sites/default/settings.php                     |
| drupal-version            | 8.6.0-alpha1                                   |
| drush-conf.0              | /var/www/vendor/drush/drush/drush.yml          |
| drush-conf.1              | /var/www/drush/drush.yml                       |
| drush-conf.2              | /var/www/docroot/sites/default/local.drush.yml |
| drush-script              | /var/www/vendor/bin/drush                      |
| drush-temp                | /tmp                                           |
| drush-version             | 9.3.0                                          |
| files                     | sites/default/files                            |
| install-profile           | microsite                                      |
| modules                   | sites/all/modules                              |
| php-bin                   | /usr/local/bin/php                             |
| php-conf.1                | false                                          |
| php-os                    | Linux                                          |
| private                   | /var/www/files-private                         |
| root                      | /var/www/docroot                               |
| site                      | sites/default                                  |
| stacks.drupal-vm.inited   | false                                          |
| stacks.dev-desktop.inited | false                                          |
| temp                      | /tmp                                           |
| theme                     | particle                                       |
| themes                    | sites/all/themes                               |
| uri                       | http://fanniemae.docksal                       |
+---------------------------+------------------------------------------------+
+--------------------------------------+-----------------------------------------------------------+
| Check                                | Problem                                                   |
+--------------------------------------+-----------------------------------------------------------+
| NodeCheck:checkNodeVersionFileExists | Neither .nvmrc nor .node-version file found in repo root. |
+--------------------------------------+-----------------------------------------------------------+

When I run the pre-commit hook and I have no *.twig files staged for commit, it still seems to hang for about 10-15 seconds on each of those steps. I'd expect it to immediately recognize that there's nothing to do and move on.

I'm investigating the slowness but figured I'd drop this issue in the meantime in case anyone already knows what's going on here.

@crittermike
Copy link
Author

Ok, the slowness is coming from this line in TwigCommand.php:

if (!is_null($fileset) && iterator_count($fileset)) {

Specifically it's the iterator_count() call. For me, when running inside a Docker container, this call takes about 20 seconds consistently, even when I have no Twig files staged for commit.

@crittermike
Copy link
Author

crittermike commented Aug 14, 2018

Here's the value of $fileset at that time, for me. https://gist.github.com/mikecrittenden/29c20e08ba9f0f6fd0dd5c83db36c881

I don't really understand the process that this is trying to go through, but it seems like there's room for improvement if we're iterating over something that gigantic when I don't have any Twig files to lint in the first place. Any thoughts?

As a side note, YAML linting completes immediately.

@crittermike crittermike changed the title Pre-commit Twig/YAML linting is slow running in Docker, even when no .yaml or .twig files have been staged for commit Pre-commit Twig linting is slow running in Docker, even when no .twig files have been staged for commit Aug 14, 2018
@crittermike crittermike reopened this Aug 14, 2018
@crittermike crittermike changed the title Pre-commit Twig linting is slow running in Docker, even when no .twig files have been staged for commit Pre-commit Twig linting is slow, even when no .twig files have been staged for commit Aug 15, 2018
@crittermike
Copy link
Author

Updated to reflect that it's slow whether running in Docker or not, because it takes a few seconds even when running locally when I don't have any Twig files staged for the current commit. It's slower in Docker but even outside of Docker it seems like the slowness is an issue.

@ba66e77 ba66e77 added Enhancement A feature or feature request 9.1.x labels Aug 17, 2018
@crittermike
Copy link
Author

crittermike commented Sep 5, 2018

Here's some example verbose output from calling tests:twig:lint:files with only a single file. Note that it takes 25 seconds running inside a Docker container, even though only a single file was passed in.

fin blt -vvv tests:twig:lint:files sample-file.twig
[debug] Drupal VM is not initialized.
Linting twig files...
[debug] Gathering filesets from annotated methods...
[debug] Calling getFilesetPhpCustomModules on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetPhpCustomThemes on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetPhpTests on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetFrontend on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetTwig on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetYaml on Acquia\Blt\Robo\Filesets\Filesets object...
Iterating over fileset files.twig...

 // OK in /var/www/docroot/sample-file.twig                 

                                                                                                                        
 [OK] All 1 Twig files contain valid syntax.                                                                            
                                                                                                                        

All Twig files contain valid syntax.
25.215s total time elapsed.

@danepowell
Copy link
Contributor

I cannot reproduce this behavior. Are you able to provide steps to reproduce starting from a fresh install of BLT 9.2.x or 10.x?

Specifically, I created a new BLT project and ran the following within DrupalVM (I don't have a Docker instance, but NFS performance in Vagrant should be similar):

blt -vvv tests:twig:lint:files sample-file.twig
Linting twig files...
[debug] Gathering filesets from annotated methods...
[debug] Calling getFilesetPhpCustomModules on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetPhpCustomThemes on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetPhpTests on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetFrontend on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetTwig on Acquia\Blt\Robo\Filesets\Filesets object...
[debug] Calling getFilesetYaml on Acquia\Blt\Robo\Filesets\Filesets object...
[info] No files were found in fileset files.twig. Skipped.
All Twig files contain valid syntax.
0.176s total time elapsed.

@danepowell danepowell added Support A support request and removed Enhancement A feature or feature request labels Mar 18, 2019
@crittermike
Copy link
Author

I'm going to go ahead and close this in that case - seems to be a project-specific problem somehow.

@bkosborne
Copy link
Contributor

bkosborne commented Jan 6, 2022

FWIW, I have this same problem. Also using Docker with NFS, and also identified the slow portion to be the call to iterator_count($fileset) in that function. Frustratingly this happens on every single commit, despite not having any twig files in the committed fileset, and doesn't look like we can disable just this check from the pre-commit script.

The fact that it's scanning anything despite there not being any files that have a .yml or .yaml extension seems like a bug, though perhaps it's best added as a separate issue, as it doesn't make much sense to me that it should do anything in this case.

The fileset that it scans is files.twig:

  /**
   * Get Twig fileset.
   *
   * @fileset(id="files.twig")
   *
   * @return \Symfony\Component\Finder\Finder
   *   finder
   */
  public function getFilesetTwig() {
    $finder = $this->getTwigFilesetFinder();
    $finder->in([$this->getConfigValue('docroot') . '/themes/custom']);
    $finder->in([$this->getConfigValue('docroot') . '/modules/custom']);

    return $finder;
  }

And we have a lot of files in those directories, so I assume that's the slow down.

@bkosborne
Copy link
Contributor

Added #4451 as related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Support A support request
Projects
None yet
Development

No branches or pull requests

4 participants