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

Use composer authoritative class maps in production build #7362

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Nov 30, 2022

Summary

Fixes #7337

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Nov 30, 2022

Plugin builds for 84f86a5 are ready 🛎️!%0A- Download development build%0A- Download production build

@thelovekesh
Copy link
Collaborator Author

thelovekesh commented Nov 30, 2022

According to the docs Authoritative class maps relieves autoloader from file lookups in case the class is not present in the class map.

Usage of classmap authoritative by autoloader
/**
 * Finds the path to the file where the class is defined.
 *
 * @param string $class The name of the class
 *
 * @return string|false The path if found, false otherwise
 */
public function findFile($class)
{
	// class map lookup
	if (isset($this->classMap[$class])) {
		return $this->classMap[$class];
	}
	if ($this->classMapAuthoritative || isset($this->missingClasses[$class])) {
		return false;
	}
	if (null !== $this->apcuPrefix) {
		$file = apcu_fetch($this->apcuPrefix.$class, $hit);
		if ($hit) {
			return $file;
		}
	}

	$file = $this->findFileWithExtension($class, '.php');

	// Search for Hack files if we are running on HHVM
	if (false === $file && defined('HHVM_VERSION')) {
		$file = $this->findFileWithExtension($class, '.hh');
	}

	if (null !== $this->apcuPrefix) {
		apcu_add($this->apcuPrefix.$class, $file);
	}

	if (false === $file) {
		// Remember that this class does not exist.
		$this->missingClasses[$class] = true;
	}

	return $file;
}

@westonruter
Copy link
Member

I'm looking at the diff of the build before and after this change, and it seems the only change is in vendor/composer/autoload_real.php with the removal of these lines:

$map = require __DIR__ . '/autoload_namespaces.php';
foreach ($map as $namespace => $path) {
$loader->set($namespace, $path);
}
$map = require __DIR__ . '/autoload_psr4.php';
foreach ($map as $namespace => $path) {
$loader->setPsr4($namespace, $path);
}

@andyblackwell Is this what you were expecting?

@westonruter westonruter added this to the v2.3.1 milestone Nov 30, 2022
@andyblackwell
Copy link

I'm looking at the diff of the build before and after this change, and it seems the only change is in vendor/composer/autoload_real.php with the removal of these lines:

$map = require __DIR__ . '/autoload_namespaces.php';
foreach ($map as $namespace => $path) {
$loader->set($namespace, $path);
}
$map = require __DIR__ . '/autoload_psr4.php';
foreach ($map as $namespace => $path) {
$loader->setPsr4($namespace, $path);
}

@andyblackwell Is this what you were expecting?

Sorry, didn't see this notification. Yes, that's expected. I downloaded the production build from the github actions comment, and can see the changes you mentioned plus the line: $loader->setClassMapAuthoritative(true);

I also see it in the development build, which typically isn't what you'd want, but I don't know your workflow. If you don't actively make edits to the actual development build after the fact, like creating/renaming classes, then that should be fine.

@westonruter
Copy link
Member

@andyblackwell Thank you for pointing that out.

@thelovekesh Could you revert the change to composer_install and instead put the change into a composer_install_prod command which is otherwise a copy of composer_install? Then in the build command, we can use a condition like the following:

amp-wp/Gruntfile.js

Lines 170 to 173 in 371fa96

if ( 'development' === process.env.NODE_ENV ) {
paths.push( 'assets/js/**/*.js.map' );
paths.push( 'assets/css/*.css.map' );
}

To decide whether we run composer_install (or composer_install_dev) or composer_install_prod here:

grunt.task.run( 'shell:composer_install' );

@thelovekesh thelovekesh self-assigned this Dec 7, 2022
Comment on lines +87 to +88
`composer install --no-dev -o${ 'development' === process.env.NODE_ENV ? '' : 'a' }`,
`composer remove cweagans/composer-patches --update-no-dev -o${ 'development' === process.env.NODE_ENV ? '' : 'a' }`,
Copy link
Member

Choose a reason for hiding this comment

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

Well done!

@westonruter westonruter merged commit bf74d0d into develop Dec 7, 2022
@westonruter westonruter deleted the enhancement/composer-autoloader-optimization branch December 7, 2022 19:36
@westonruter westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize Production Composer Build to reduce needless file lookups
3 participants