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

[Validator] Unnecasarry validation metadata disk reads #39945

Closed
umpirsky opened this issue Jan 22, 2021 · 6 comments · Fixed by #44742
Closed

[Validator] Unnecasarry validation metadata disk reads #39945

umpirsky opened this issue Jan 22, 2021 · 6 comments · Fixed by #44742

Comments

@umpirsky
Copy link
Contributor

umpirsky commented Jan 22, 2021

Symfony version(s) affected: >= 4.4

Description

Looks like this code causes disk reads, when rendering a form, this code is called, even when validation does not take place:

#0  Symfony\Component\Validator\Mapping\Loader\FileLoader->__construct() called at [vendor/symfony/symfony/src/Symfony/Component/Validator/ValidatorBuilder.php:326]
#1  Symfony\Component\Validator\ValidatorBuilder->getLoaders() called at [vendor/symfony/symfony/src/Symfony/Component/Validator/ValidatorBuilder.php:352]
#2  Symfony\Component\Validator\ValidatorBuilder->getValidator() called at [app/cache/dev/ContainerVvqCaGq/appAppKernelDevDebugContainer.php:6813]
#3  ContainerVvqCaGq\appAppKernelDevDebugContainer->getValidatorService() called at [app/cache/dev/ContainerVvqCaGq/getForm_TypeExtension_Form_ValidatorService.php:9]
#4  require(app/cache/dev/ContainerVvqCaGq/getForm_TypeExtension_Form_ValidatorService.php) called at [app/cache/dev/ContainerVvqCaGq/appAppKernelDevDebugContainer.php:2818]
#5  ContainerVvqCaGq\appAppKernelDevDebugContainer->load() called at [app/cache/dev/ContainerVvqCaGq/getForm_RegistryService.php:228]
#6  ContainerVvqCaGq\appAppKernelDevDebugContainer->{closure}() called at [vendor/symfony/symfony/src/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php:56]
#7  Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension->getTypeExtensions() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormRegistry.php:120]
#8  Symfony\Component\Form\FormRegistry->resolveType() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormRegistry.php:94]
#9  Symfony\Component\Form\FormRegistry->getType() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormRegistry.php:127]
#10 Symfony\Component\Form\FormRegistry->resolveType() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormRegistry.php:94]
#11 Symfony\Component\Form\FormRegistry->getType() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:58]
#12 Symfony\Component\Form\FormFactory->createBuilder() called at [vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:30]
#13 Symfony\Component\Form\FormFactory->create() called at [vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php:324]

Looks like ValidatorBuilder::getLoaders() only creates loaders and constructors fire disk reads, even if FileLoader::loadClassMetadata() is not called.

Possible Solution
Check is_file(), is_readable(), stream_is_local() only when validation is executed (FileLoader::oadClassMetadata() is called).
This can be further reduced with caching, so it des not check files on each HTTP request.

@umpirsky umpirsky added the Bug label Jan 22, 2021
@umpirsky umpirsky changed the title [Validation] Unnecasarry validation metadata disk reads [Validator] Unnecasarry validation metadata disk reads Jan 22, 2021
@YaFou
Copy link
Contributor

YaFou commented Jan 22, 2021

Hum, interesting report... This is true, the getLoaders method is called two times. However I don't know if is_file(), is_readable(), stream_is_local() functions take lot of time. Moreover, I think that PHP has a file info cache that improves the performance of these functions. Perhaps, I need more information.

@umpirsky
Copy link
Contributor Author

umpirsky commented Jan 28, 2021

Just to be clear, this issue is not about taking time or slowness. It's about disk reads that are not necessary.

@xabbuh xabbuh added Performance and removed Bug labels Feb 17, 2021
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@umpirsky
Copy link
Contributor Author

Nope.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 25, 2021

Please check #44742
I suppose you ran some benchmark before opening this ticket?
Can you please share some data about the way this improve things?

@umpirsky
Copy link
Contributor Author

Let me try...

nicolas-grekas added a commit that referenced this issue Jan 31, 2022
…nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[Validator] check for file existence lazily in loaders

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39945
| License       | MIT
| Doc PR        | -

Commits
-------

9135405 [Validator] check for file existence lazily in loaders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants