Improve performance of script handlers #12456
Conversation
|
For the case in #8587 (comment), I think another improvement could be done: |
yeah I thought about this but it is most often used for binaries which are anyway now skipping the autoloader completely, so I don't think it is worth detecting this special case (especially as it is maybe not so easy, we lack that context inside EventDispatcher).. As creating a single autoloader is often relatively cheap, the main problem is when it keeps recreating them. The other issue is that we do not actually load the project's autoloader, so this would fail if we do not create an autoloader inside run-script. |
Seldaek
force-pushed
the
event_autoload_perf
branch
from
f983c75 to
8ead554
Compare
Only create autoloaders when needed, and avoid creating duplicate autoloaders Refs composer#8587
Seldaek
force-pushed
the
event_autoload_perf
branch
from
8ead554 to
3372a31
Compare
|
I am about 98% sure this broke our (outdated) scripthandler, which had in it a require to a class being installed with composer Like so: use DrupalFinder\DrupalFinder;
use Symfony\Component\Filesystem\Filesystem;
// ...
$fs = new Filesystem();
$drupalFinder = new DrupalFinder();after CI got 2.9.0: |
|
A workaround for Drupal is to remove the |
| $generator->setDevMode($event->isDevMode()); | ||
| $hash .= $event->isDevMode() ? '/dev' : ''; | ||
| } | ||
| $hash = hash('sha256', $hash); |
There was a problem hiding this comment.
Is it worth just adding in the event type here? As in, if it's pre or post - as inherently the package list is highly likely to be different between them. This would ensure the post hook generates a new autoloader.
I think issue is the package list for local repository is technically the same before an install as it is after - the only difference is that the vendor folder exists after - which from an autoloader perspective I guess is completely different? Perhaps the above is the best way to fix the issue? Or if there's perhaps a flag somewhere to say if install has been completed or not?
|
Awesome. Also for the record. My comment reads a bit, eheh, short, when I tried to jot this down before running out the door. Sorry about that. I also have the opinion that the composer team is amazing, just to state that ❤️ |
|
...and can I just also point out that projects that are using that scripthandler usually do that because they have inherited the setup from the good old drupal-composer project. Now we have official ways of doing it, and that probably does not break in this way (at least I think it would not do that) In my case, the script is almost completely useless, so it's a good occasion to clean them up a bit 🤓 |
No worries, just wish more people tested snapshots or RCs as that'd reduce the need for such stressful moments, but aside from that all good :D |
6 participants
Only create autoloaders when needed, and avoid creating duplicate autoloaders.
In pathological cases like declaring a post-package-install script as
"Foo::bar", it would classmap-scan the whole project on every package being installed, causing 200ms of delay at every package. Trying this on a repo here this PR improves a fresh install time from 38 down to 9seconds, which is quite dramatic. I suppose other cases where large classmaps or listener-heavy plugins are present might also see significant speedups.Targetting 2.9 though because it does carry some risk of regression in case I missed a corner.
Refs #8587