Exclude unused validator spec classes from amp-toolbox-php

[westonruter]

Bug Description

The optimized autoloader generates a class map with many many files. The majority of these files (936 out of 1,375 or 68%) come from the validator spec files in amp-toolbox-php located in vendor/ampproject/amp-toolbox/src/Validator/Spec/. I received a report that the ClassLoader execution time was slow perhaps due to the large size of the class map. We can improve the runtime performance as well as decrease the number of files in the plugin by excluding these files from the build. The validator spec files are not being used by the plugin.

Expected Behaviour

Unused validator spec classes should be omitted from the plugin.

Screenshots

No response

PHP Version

No response

Plugin Version

2.3.0

AMP plugin template mode

Standard, Transitional, Reader

WordPress Version

No response

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

[thelovekesh]

@westonruter This patch removes above mentioned Spec files from class map.

diff --git a/composer.json b/composer.json
index f7fa32a78..433891905 100644
--- a/composer.json
+++ b/composer.json
@@ -53,6 +53,10 @@
     ],
     "files": [
       "includes/bootstrap.php"
+    ],
+    "exclude-from-classmap": [
+      "/vendor/ampproject/amp-toolbox/src/Validator/Spec/",
+      "/vendor/ampproject/amp-toolbox/src/Validator/Spec.php"
     ]
   },
   "autoload-dev": {

To remove these files from our build, we can add vendor/ampproject/amp-toolbox/src/Validator/Spec/ in the exclude file patterns in build runner(Grunt).

---

EDIT:

We can improve the runtime performance as well as decrease the number of files in the plugin by excluding these files from the build.

Just tried excluding Spec files from the build and it results in a fatal error but removing these files from the class map works fine.

Fatal Error

[westonruter]

The fatal error is due to some code in the plugin using a spec class?

[thelovekesh]

Not sure yet 🤔 . I am just curious why it's creating an instance of Spec when we are not using it in the plugin. Also removing vendor/ampproject/amp-toolbox/src/Validator/Spec/ from the class map works fine.

[andyblackwell]

Those Validator\Spec files look like they're used during html optimization, and not just test specs (if that's what you were thinking), so you won't be able to remove them and shouldn't exclude them from the classmap.

https://github.com/ampproject/amp-toolbox-php/search?q=optimizer+validator%5Cspec&type=code

It still worked when you only removed from the classmap due to how you do the composer build currently without --classmap-authoritative, so composer still did fallback lookups and found the classes via standard psr-4.

The performance issues originally noted were more likely due to the issue I brought up, having to do more with this fallback class lookup composer does when built without --classmap-authoritative. I put a workaround in place on our sites and it reduced 7-8 thousand needless file_exist() class lookups to virtually none afterward, granted there were a few other plugins with this same issue. This sped up TTFB in production on our sites when hitting the origin server by 100-200ms.

[westonruter]

Those Validator\Spec files look like they're used during html optimization, and not just test specs (if that's what you were thinking), so you won't be able to remove them and shouldn't exclude them from the classmap.

Got it. I see Validator\Spec mentioned in these files:

vendor/ampproject/amp-toolbox/src/Dom/Document.php
vendor/ampproject/amp-toolbox/src/Optimizer/Configuration/TransformedIdentifierConfiguration.php
vendor/ampproject/amp-toolbox/src/Optimizer/TransformationEngine.php
vendor/ampproject/amp-toolbox/src/Optimizer/Transformer/AutoExtensions.php
vendor/ampproject/amp-toolbox/src/Optimizer/Transformer/TransformedIdentifier.php

Specifically these usages:

use AmpProject\Validator\Spec;
use AmpProject\Validator\Spec\AttributeList\GlobalAttrs;
use AmpProject\Validator\Spec\CssRuleset\AmpNoTransformed;
use AmpProject\Validator\Spec\CssRuleset\AmpTransformed;
use AmpProject\Validator\Spec\Error\DevModeOnly;
use AmpProject\Validator\Spec\Error\DisallowedAttr;
use AmpProject\Validator\Spec\Error\DisallowedManufacturedBody;
use AmpProject\Validator\Spec\Error\DuplicateAttribute;
use AmpProject\Validator\Spec\Error\DuplicateUniqueTag;
use AmpProject\Validator\Spec\Error\InvalidAttrValue;
use AmpProject\Validator\Spec\Error\InvalidDoctypeHtml;
use AmpProject\Validator\Spec\Error\MandatoryAttrMissing;
use AmpProject\Validator\Spec\SpecRule;

So if it ends up being that we should still exclude unused classes, perhaps we can just do so for AmpProject\Validator\Spec\Tag, AmpProject\Validator\Spec\AttributeList, AmpProject\Validator\Spec\Error, etc.

[andyblackwell]

to be honest, I would highly recommend not messing with excluding these files to avoid unexpected breakage
especially since even those you mentioned at the end to still exclude, I can see are all still actually used in that library and would result in breakage

in my opinion, the performance boost to be gained here is going to be adding --classmap-authoritative to your build like mentioned in #7337, lowest hanging fruit with the most return

[westonruter]

OK, agreed. I'm removing this issue from the milestone and deprioritizing.

[thelovekesh]

@westonruter Do we still need this as something similar has been fixed in #7337.

[westonruter]

No, we can close this.