There are a lot of Childs willing to be replaced by Children

@pborreli renamed

@stof What is the status of this PR? We need it finished for #4605 be mergeable.

@Nomack84 the issue is performances, probably due to the fact that clearstatcache has been added. see the discussion in the old PR.

@stof shouldn't clearstatcache be done either once / instance or if it's a long running process let the user control when he wants to clear it?
This way we should be able to fix all the performance issues, if they are generated by that.

What do you think?

I checked out @stof branch tonight. I ran the whole test suite and am trying to figure out how to reproduce this slowness reported from before. Can anyone elaborate on just how much slower this code makes things and how I can see it for myself?

As it stands now I'm running the whole test suite and it looks like the time difference is minimal. The current implementation is slower seconds over the course of the entire test suite. That does not sound as dire as I expected, though, so I'm hoping for tips on how to better see the problem.

Results from commit 875e0e2 (last commit before this PR)

Time: 01:57, Memory: 422.25Mb
Time: 01:58, Memory: 422.25Mb
Time: 02:02, Memory: 422.25Mb

Current state of this PR

Time: 01:57, Memory: 422.75Mb
Time: 02:00, Memory: 422.75Mb
Time: 02:05, Memory: 422.75Mb
Time: 02:06, Memory: 422.75Mb

After some minor tweaks

Time: 01:58, Memory: 422.75Mb

Blindly commenting out the clearstatecache calls from DirectoryResource

Time: 01:54, Memory: 422.50Mb
Time: 02:01, Memory: 422.50Mb

For a period of time phpunit was leaving a spare php process laying behind. It was using 100% CPU. I was dumped to the prompt normally so I didn't notice it at first. After playing around with various branches for awhile it "just stopped doing it." Does this happen sometimes while running the whole test suite?

The problem presented itself by increase in run time... from 3-4 minutes (almost double). While this was a problem, php would stick around regardless of which branch I tried to test of.

People who reported the slowness were people using the full-stack framework. So, I suppose that testing performance of the standard edition with and without the patch should reveal the performance issue.

If there's any demo setup that I could use as a base for testing I'll gladly do the debugging/performance analysis of this issue but unfortunately I don't get it what should be installed where and how and so on and I don't really have the patience to read thru all of issues/comments, sorry for being lazy.

I've took the branch from @stof rebased it against master and put it in a Symfony2 Standard master version with composer update --dev, a couple of entities and made all the tune up for performance. I've found the following:

• the branch from stof is almost always a bit faster, a couple of seconds, 5-7 seconds that master branch;
• there's a BC break in terms of running the anything that uses TWIG as output, I suspect because of the collectors. It just breaks with something like this: PHP Fatal error: appDevDebugProjectContainer::getDoctrine_Orm_DefaultEntityManagerService(): Failed opening required '/var/www/personal/symfony-stof/app/cache/dev/jms_diextra/doctrine/EntityManager_50e494e43e9df.php' (include_path='.:/usr/share/php:/usr/share/pear') in /var/www/personal/symfony-stof/app/cache/dev/appDevDebugProjectContainer.php on line 263
• it runs ok if I don't do a return new Response('demo'); in the controller, which was used to run the benchmarks.

Thanks to @simensen for getting this up and running.

@fabpot where was the slowness reported? Any reproducible test case? The tests aren't showing something spectacular and the Symfony2 Standard edition seems to be running, aside for the above mentioned glitch which I'm not even sure where to start debugging it right now, I'll check in the coming days.

It would be a shame to leave this out especially after all the hard work being done for it and the already missed 2.1 release.

Let me know how can I further assist on this.

My test configuration:

• Command used: ab -c 10 -n 100000 --url--
• CPU: Intel Core i7 2630QM @2Ghz
• Memory: 8GiB DDR3
• DISK: SSD OCZ Vertex 4 128 GiB
• OS: Ubuntu 12.10
• Kernel: Linux 3.6.3-generic
• PHP: 5.4.6-1ubuntu1 mod_dso used
• TWIG: using TWIG extension for all tests
• APC: 3.1.13 with apc.stats = 0
• Apache: 2.2.22
• Symfony: branch from @stof + rebase on master / prod env, all bundles enabled
• URL tested: Custom return demo string

This change: https://github.com/symfony/symfony/pull/5744/files#L1L32 causes creation of broken meta file in standard edition, efficiently killing Symfony2. I have no idea why as of yet :/

Also this error was a cause of merge revert: #4626. It's a single blocker of this PR and ResourceWatcher as consequence. I'm out of ideas here.

Oooor. We can revert this particular line back to:

        $this->resource = realpath($resource);

And we're ready to merge. @stof @fabpot what do you think?

Update: nevermind. ResourceWatcher ability to track unexisting (yet?) files relies on this change, cuz tracking resource which FileResource::getResource() will return false will just not work.

In case if someone interested in bug, here's what's happening when Symfony2 creates cache:

1. It tries to generate meta for call cached resources (files). It does this by caching ResourceInterface instances into one big file
2. Most of the cache resources exist, causing no difference - realpath() returns path to resource, which gets serialised by FileResource::serialize()
3. Most, except .../app/cache/dev_new/kernel.tmp. This file sometimes (always?) doesn't exist. And that's where the difference comes into play - old behaviour (realpath()ing everything) was causing FileResource::$resource to be set to false. This PR prevents FileResource::$resource from being set to false by setting unexisting file path instead.
4. FileResource::$resource gets serialized and cached together with other resources by ConfigCache. And here comes the bug. false value gets serialized just ok, but unexisting file path to kernel.tmp not.
5. Kernel tries to read and unserialize meta file, which is broken in case of kernel.tmp and causes unserialize exception.

Way to reproduce:

1. Replace $this->resource = realpath($resource); with $this->resource = file_exists($resource) ? realpath($resource) : $resource in the FileResource::__construct.
2. Remove cache - rm -rf app/cache.
3. Clear cache - app/console ca:c
4. Call any Symfony2 console command and you'll get exception.

Sorry for hopping on this so late. I wasn't aware of ResourceWatcher until recently and was very excited when I learned about it. I am really looking forward to it - great job @everzet and all involved!

Regarding this particular PR and changes to the Config/ResourceInterface, I'd like to voice some concerns though.

I think this PR blends changes that are necessary to support "ResourceWatching" into an interface that has a different purpose right now. This violates the segregated interface principle.

I tried to explain what ResourceInterface currently is (and IMHO should be even more clearly) in issue #7176 .
As there are only filesystem-backed ResourceInterface implementations right now the difference might be hard to see.

In particular

• getId(), getModificationTime() and exists() in the changed ResourceInterface are only relevant for ResourceWatch'ing, not for ConfigCaching - they have no meaning there.
• Having to clearstatcache() is an implementation detail relevant for ResourceWatching, but gets (performance-wise) in the way of validating a ConfigCache. (Is it even necessary to have it when using the inotify-based state tracker?)
• It makes the Config component bring along stuff that is ResourceWatcher specific, which I consider a dependency the wrong way round.

I do not say that there could not be an implementation of a "filesystem file thing" or "directory thing" (avoiding "Resource" here) that supports both ResourceInterface (for ConfigCache'ing) and ResourceWatch'ing, but I think both are different concerns and should be handled as such. Whether such an implementation should be in the Config component is yet another question.

ConfigCache might be used with more general types of Resources (as of Loaders, they even support loading from Closures). I don't think is feasible to use this general notion of "Resources" for ResourceWatching though (at least not in all cases).

The one link I see between ResourceWatcher and ConfigCache is the idea of using the former to watch and expunge the cache of the latter, but I don't know how that could happen during a regular (HTTP) kernel request at all.

Closing this PR. The new features added in the ResourceInterface are breaking BC and are not needed anymore as the ResourceWatcher component won't be merged in favor of https://github.com/henrikbjorn/Lurker shipping the code standalone