Skip to content

Reset Profiler data after each request for PHP-PM #18244

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

Closed
marcj opened this issue Mar 20, 2016 · 11 comments
Closed

Reset Profiler data after each request for PHP-PM #18244

marcj opened this issue Mar 20, 2016 · 11 comments
Assignees
Labels

Comments

@marcj
Copy link

marcj commented Mar 20, 2016

Hi!

I'm currently trying to optimize Symfony usage in https://github.com/php-pm/php-pm.

One of those optimizations is to get the correct debugging information in the profiler. Currently after each request only the execution time is correctly calculated. All other stuff like logs, template render times, query logging etc are not reseted after Kernel::terminate(). What we currently do is along these lines: https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Symfony.php#L71-L100
I know it's a hack and don't get any support through semver as we hack into the object, but for the moment the only fast way to get things working. If it's working we could provide a pull requests to get this inside Symfony with well defined interfaces.

First Request:
screen shot 2016-03-21 at 00 24 13

9th Request
screen shot 2016-03-21 at 00 20 13

Any ideas?

//Edit
We get for each new request a new Debug-Token, as expected.

@marcj marcj changed the title Reset Profiler data for PHP-PM Reset Profiler data after each request for PHP-PM Mar 20, 2016
@linaori
Copy link
Contributor

linaori commented Mar 21, 2016

I'm unfamiliar with what you're doing but if my assumption is correct, it means that you don't close the request/terminate symfony? Could you explain a bit more about the challenge you're facing?

@marcj
Copy link
Author

marcj commented Mar 21, 2016

it means that you don't close the request/terminate symfony

What do you mean with 'close the request' or 'terminate symfony'? We do close the request by calling Kernel::terminate($request, $response);, but we do not shutdown the Kernel, which should not be necessary as we need the kernel for further requests.

@stof
Copy link
Member

stof commented Mar 21, 2016

@marcj not shutting down the kernel also means that you reuse the same Container instance all the time. This is what could cause issues in several places (and it is exactly what causes issues here). Several collector services are stateful, and so would keep the data and mix them with the next request in the profiler. You will also have issues with the FingersCrossedHandler of Monolog for instance.

Rather than trying to list all places where a cleanup would be necessary at the end of the request (which is quite painful as there is no such listing today, and any bundle could contain such a place), have you tried to look at the performance impact of calling $container->reset() to perform a complete cleanup (you will still need to reset the start time though) ? It may be much easier for now, if the perf impact is acceptable.
Note that ResettableContainerInterface is only available as of Symfony 2.8, but you can detect whether the container supports the feature by checking whether it implements the interface.

@marcj
Copy link
Author

marcj commented Mar 21, 2016

Instead of doing https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Symfony.php#L90-L99 I tried $app->getContainer()->reset() but this breaks with the exception You have requested a synthetic service ("kernel"). The DIC does not know how to construct this service. after the second request.
I also tried $app->shutdown(), but this makes it terrible slow again.

@stof
Copy link
Member

stof commented Mar 21, 2016

ah right, the kernel sets itself only during the initial creation. Try this then:

$app->getContainer()->reset();
$app->getContainer()->set('kernel', $app);

@marcj
Copy link
Author

marcj commented Mar 21, 2016

Rather than trying to list all places where a cleanup would be necessary at the end of the request (which is quite painful as there is no such listing today, and any bundle could contain such a place)

It is indeed painful and quite a bit of work, but I think it's the only way to keep this whole approach alive and to keep the good performance. But I believe it's time to make this list now :) Maybe PHP-PM is the starting shot for it.

I just tried the latest comment with set('kernel', $app), this works, but makes it again very slow. I guess it's almost equal to shutdown except the boot calls to all bundles (which is important when you reset the container, at least in one of my applications). I guess resetting the container is not the right way to go, although I'd be perfect if it wouldn't hurt the performance so much as it could be more compatible. However, I guess I need to investigate deeper.

Any other hints where I can reset the profiler?

@marcj
Copy link
Author

marcj commented Mar 22, 2016

Ok, I know now why those stats don't get reseted. The DataCollectorInterface has only the method collect which most DataCollector implementations have in a way implemented that they don't delete the stuff when collect is called. This would be currently the only way to keep the memory clean, but would not be perfect, as a second call would return reseted data. A new method DataCollectorInterface::clean would solve that issue, but would obviously take too much time until all implementations have updated, so the only way is currently to reset the services and dependencies in the container.

@marcj
Copy link
Author

marcj commented Mar 22, 2016

I have done now some really nasty hacks, but works so far.
php-pm/php-pm-httpkernel@d6199a5#diff-4e4aadfdc2edbd7dfe3479c6259178eaR86

What do you guys think about making this more pretty by introducing some new API methods to the DataCollector stuff? FrameworkBundle/ProfilerListener could call a method like clear on onKernelTerminate which basically cleans all data collectors.

@linaori
Copy link
Contributor

linaori commented Mar 22, 2016

You could solve it with something like DataCollectorResetInterface::reset().

@inso
Copy link

inso commented Mar 22, 2016

@marcj maybe you can try to reset state of data collectors using reflection?

@fabpot
Copy link
Member

fabpot commented Mar 22, 2016

@marcj Making Symfony work better with solution like php-pm is always a good idea. I spent a lot of time a few years back to make sure that Symfony does not leak memory, and we should continue this work. Adding some clean method in the master branch would not solve your immediate problem but makes Symfony stronger in the long term, which is great.

@nicolas-grekas nicolas-grekas self-assigned this Jul 28, 2017
fabpot added a commit that referenced this issue Aug 31, 2017
…EFERENCE (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As spotted in #23984 and #18244, supporting php-pm and the likes requires us to have a way to reset services, typically on `kernel.terminate`. Yet, only actually instantiated services need to be resetted.

Knowing if a service is initialized or not is doable for public ones thanks to `Container::initialized()`. But for private ones, there is no way to achieve it.

I propose to add a new way to reference services via this new const. When a reference has this behavior, two things happens:
- at runtime, if the referenced service is not initialized, it behave the same as "ignore on invalid", pretending the service doesn't exist
- at compile time, such references are weak, meaning their referenced services *can* be removed, but *cannot* be inlined

Commits
-------

0db3358 [DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE
fabpot added a commit to twigphp/Twig that referenced this issue Sep 27, 2017
This PR was merged into the 1.x branch.

Discussion
----------

Added pssibility to reset the profiler

In order to implement symfony/symfony#18244 properly, I needed the ability to reset a Twig profile.

Commits
-------

ea922bb Added pssibility to reset the profiler.
fabpot added a commit that referenced this issue Oct 5, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle][HttpKernel] Reset profiler

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #18244
| License       | MIT
| Doc PR        | N/A

This PR adds the ability to reset the profiler between requests. Furthermore, the profiler service has been tagged with the new `kernel.reset` tag from #24155. For this, I had to readd the ability to define multiple reset methods for a service.

Note: This PR requires twigphp/Twig#2560.

Commits
-------

8c39bf7 Reset profiler.
@fabpot fabpot closed this as completed Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants