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

Jump to bottom

Remove non-autoloadable classes from preload #44380

Open
wants to merge 1 commit into
base: 6.1
Choose a base branch
from
Open

Remove non-autoloadable classes from preload #44380

wants to merge 1 commit into from
+135 −7

Conversation

@jderusse
Copy link
Member

@jderusse jderusse commented Dec 1, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? mp
Tickets Fix #44364
License MIT
Doc PR

When a class is required in a preloaded file, all the type hinted properties have to be auto-loadable. (sees discussion in linked issue)

This could be an issue for classes like BodyRenderer that have a property typehinted with an optional dependency.

symfony/src/Symfony/Bridge/Twig/Mime/BodyRenderer.php

Lines 27 to 39 in 02b40c0

private HtmlConverter $converter;
public function __construct(Environment $twig, array $context = [])
{
$this->twig = $twig;
$this->context = $context;
if (class_exists(HtmlConverter::class)) {
$this->converter = new HtmlConverter([
'hard_break' => true,
'strip_tags' => true,
'remove_nodes' => 'head style',
]);
}

Sometime, the relation is tricky. For instance RedisAdapter uses RedisTrait that have a property $redis typehinted RedisClusterProxy that have a property typehinted Predis\ClientInterface => but Predis is an optional dependency

When dumping the Preload classMap, this PR now check if all the properties are auto-loadable.
@carsonbot carsonbot added this to the 5.3 milestone Dec 1, 2021
@jderusse jderusse force-pushed the preload branch 3 times, most recently from ea33197 to 83dbe08 Dec 1, 2021
nicolas-grekas
nicolas-grekas reviewed Dec 1, 2021
View changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited

I really like the idea!

I think the current state of the implementation won't work: shouldn't a class that depends on a non-preloadable class also be flagged as non-preloadable?

I also have one fear: in practice, how many classes will this exclude from preloading (especially considering the previous point?) The more typed properties we use, the less classes will be preloaded.

Or maybe we shouldn't be concerned because PHP 8.1 doesn't have the issue, and preloading is also mostly dead since 8.1. We could even consider deprecating our dedicated logic for it.

jderusse
jderusse reviewed Dec 1, 2021
View changes
src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated
}
$this->preloadableCache[$class] = true; // prevent recursion

if (!class_exists($class) && !interface_exists($class, false) && !trait_exists($class, false)) {
Copy link
Member Author

@jderusse jderusse Dec 1, 2021

Choose a reason for hiding this comment

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

beware, switched from class_exists($class, false) to class_exists($class)

@jderusse jderusse force-pushed the preload branch 3 times, most recently from 8c47b47 to ca03364 Dec 1, 2021
@stof
Copy link
Member

@stof stof commented Dec 1, 2021

preloading is also mostly dead since 8.1

@nicolas-grekas what do you mean with this sentence ?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 1, 2021

I mean that preloading might not be worth the trouble it is to maintain and setup anymore...

@jderusse
Copy link
Member Author

@jderusse jderusse commented Dec 1, 2021

I think the current state of the implementation won't work: shouldn't a class that depends on a non-preloadable class also be flagged as non-preloadable?

Indeed, I missed 3 points:

  1. The Preloader browse the classes, properties and methods parameters to discover more classes.

This is now fixed by doing the ~same thing in PHPDumper.

  1. The preloader uses get_declared_classes to browse more classes

This one is tricky because, in PhpDumper, I can't easily do the same for each discovered class. That would require a tokenizer, or calling get_declared_classes in an isolated process => big overhead.

I fixed it by pre-filling the $preloaded parameter of the Preloader to prevent it to browse the classes I known to be not pre-loadable.

  1. The generated .preload file is full of require_once included in the Dumped Container

I have no choice but removing the container from the .preload.php file. (in my case, the code preloaded 2300 files before, and 1900 after that change)

@jderusse jderusse force-pushed the preload branch 3 times, most recently from e77efd2 to 4c87358 Dec 2, 2021
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 3, 2021
Closed
@fabpot
Copy link
Member

@fabpot fabpot commented Dec 7, 2021

This one is quite important as currently, Symfony 6.0 is broken when using pre-loading.

@fabpot fabpot mentioned this pull request Dec 7, 2021
Merged
@fabpot
Copy link
Member

@fabpot fabpot commented Dec 7, 2021

See #44494 for an alternative

@fabpot
Copy link
Member

@fabpot fabpot commented Dec 8, 2021

Closing in favor of #44494

@fabpot fabpot closed this Dec 8, 2021
@jderusse
Copy link
Member Author

@jderusse jderusse commented Dec 8, 2021

reopening: Removing typehinted property in symfony/symfony: 6.0 is a workaround for our internal classes, but the bug still exists for other dependencies.

The current implementation of the preloads script discovers all classes used by the end-user. Every third-party package/bundle with type hinted property will hit the same bug.

Worst case, this PR should be merged in 6.1 and type-hint re-added.

@jderusse jderusse reopened this Dec 8, 2021
fabpot added a commit that referenced this issue Dec 9, 2021
@fabpot
bug #44494 Remove FQCN type hints on properties (fabpot)
0a13eba 
This PR was merged into the 6.0 branch.

Discussion
----------

Remove FQCN type hints on properties

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Closes #44364
| License       | MIT
| Doc PR        | n/a

This is an alternative for #44380.
Basically, we are removing all property type hints on 6.0 and reverts that for 6.1.

Commits
-------

52aaa56 Remove FQCN type hints on properties
@fabpot
Copy link
Member

@fabpot fabpot commented Dec 11, 2021

This one should be rebased on 6.1 then.

@fabpot fabpot removed this from the 5.3 milestone Dec 11, 2021
@fabpot fabpot added this to the 6.1 milestone Dec 11, 2021
@jderusse jderusse changed the base branch from 5.3 to 6.1 Dec 11, 2021
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 13, 2021

The preloader uses get_declared_classes to browse more classes

This one is tricky because, in PhpDumper, I can't easily do the same for each discovered class. That would require a tokenizer, or calling get_declared_classes in an isolated process => big overhead.

Could we fix this by registering an autoloader that only collects autoloaded symbols before the real autoloader?

The generated .preload file is full of require_once included in the Dumped Container

I have no choice but removing the container from the .preload.php file. (in my case, the code preloaded 2300 files before, and 1900 after that change)

You mean in the initializer that is called on the first call to set() since #44010, right?
We might be able to fix this by not calling set() but listing the files/classes in the preload script instead, isn't it?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 13, 2021 •
edited

What about another alternative: deprecate generating the preload script at all?

Since 8.1, preloading provides almost no benefits to me.

Preloading is (was?) also sensitive to "over-preloading" (aka preloading too many classes, making it under-perform due to hash table overhead), so that any benefits provided by preloading might easily be canceled by having it not properly configured. And since configuring it is quite difficult, the gain might not be reachable in practice.

Also, I guess 8.1 is able to cache the same class/symbol for different paths, while preloading cannot.

I'd be interested in knowing @nikic's thoughts on the topic!

Or we could make the preloading script extremely simpler, eg preloading only composer+some very basic stuffs.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 13, 2021

BTW, would it possible to turn that fatal error into a warning on PHP 8.0, as a bugfix?

@nikic
Copy link
Contributor

@nikic nikic commented Dec 16, 2021

BTW, would it possible to turn that fatal error into a warning on PHP 8.0, as a bugfix?

Nope, the fatal error is required for correctness in 8.0.

As to preloading in general, the usefulness is greatly diminished with 8.1, but I don't really have data on whether there are any cases where it still makes sense.

@scheb scheb mentioned this pull request Jan 4, 2022
Closed
@wouterj
Copy link
Member

@wouterj wouterj commented Feb 9, 2022

As we've added the type to properties again in the 6.1-dev branch, what is needed to finish this PR? Is there anything I (or the community) can do to help move this forward?

@nicolas-grekas nicolas-grekas mentioned this pull request Feb 9, 2022
Open
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas

@stof stof

@derrabus derrabus

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
6.1
Linked issues

Successfully merging this pull request may close these issues.

Typehinted properties on missing class with preloading
8 participants
@jderusse @stof @nicolas-grekas @fabpot @nikic @wouterj @derrabus @carsonbot