[FrameworkBundle][HttpKernel] Reset profiler #24289
Conversation
4ac4114 to
55d550c
Compare
UPGRADE-3.4.md
Outdated
| --------------- | ||
|
|
||
| * Implementing `TraceableEventDispatcherInterface` without the method | ||
| `clearCalledListeners()` is deprecated and will be unsupported in 4.0. |
There was a problem hiding this comment.
If we can remove this deprecation, that'd be better IMHO, we don't this method in the interface to method. The tag doesn't need it, and when we wire the service, we know their exact type, so their set of methods also, isn't it?
(same for the other interfaces).
If we need a contract for a "reset" method, I'd suggest adding a dedicate interface for it instead (there could be one per component when needed, didn't look precisely)
There was a problem hiding this comment.
The intention behind adding those methods to the interfaces was that each of them is designed to collect request-related data. There should not be a use-case of implementing them without the possiblity of resetting that data.
As it is implemented right now, the data collectors reset any helper classes they use for collecting their data. This way, you can easily reset the profiler, even if you're not using the kernel.reset feature.
| @@ -46,6 +46,16 @@ public function countErrors() | |||
| } | |||
|
|
|||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function clear() | |||
There was a problem hiding this comment.
should be called "reset" for consistency
There was a problem hiding this comment.
Actually, I chose clear because that's how monolog calls similar functionality.
| $profilerDefinition = $container->getDefinition('profiler'); | ||
| $profilerDefinition->addTag('kernel.reset', array('method' => 'reset')); | ||
| if ($config['collect']) { | ||
| $profilerDefinition->addTag('kernel.reset', array('method' => 'enable')); |
There was a problem hiding this comment.
Can't this be handled in the reset method itself instead? We should try harder to not have to call several methods to reset (and thus revert the multi-tags possibility).
There was a problem hiding this comment.
If I wanted to do this, the profiler would need to know its initial state (enabled or disabled) so it can reset itself to that state. I don't feel comfortable with saving that information inside the profiler.
There was a problem hiding this comment.
ok, still look suspicious to me:
in a kernel loop, re-enabling at reset time means collecting timings from end of previous request to end of next request, isn't it? But next request might come very long after, so timings won't mean anything.
Btw, the Twig profiler has the same issue, don't you think? I mean, calling "enter()" at reset time also?
Shouldn't this re-enabling happen at kernel.request time instead?
| @@ -214,6 +214,11 @@ public function getNotCalledListeners() | |||
| return $notCalled; | |||
| } | |||
|
|
|||
| public function clearCalledListeners() | |||
There was a problem hiding this comment.
should be called "reset"
| @@ -44,6 +44,17 @@ public function collect(Request $request, Response $response, \Exception $except | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function reset() | |||
| { | |||
| // TODO: Twig PR pending. Update dependencies accordingly. | |||
There was a problem hiding this comment.
PR on Twig has been merged now and new releases are out for both 1.x and 2.x versions.
There was a problem hiding this comment.
Dependencies updated, comment removed.
cacbb30 to
d5f0646
Compare
|
I've rebased the branch in order to resolve merge conflicts. |
|
@nicolas-grekas Lets continue the discussion from #24289 (comment) here.
Since the event handler runs very late on the
Timings are done by the stopwatch. As of #24193, we're resetting the stopwatch, but that only clears all collected timings from it. Resetting does not start a new timing. The only remaining issue would be the My best guess would be to move setting
That's what happens in the Twig Profile's constructor and it would be pretty strange if we leave the class after reset in a different state than after construction. Also, the Twig profile that we have in the service container is (as far as I can tell) only used for appending child profiles to it, and not for timing purposes. |
|
I've tested this branch with the Symfony demo application and PHP-PM. I've configured PHP-PM to use a single worker to make sure I'm really testing my reset feature. I've also removed the workaround code from PHP-PM's Symfony adapter locally: So far, the profiler looked okay to me. |
|
Just pushed https://github.com/nicolas-grekas/symfony/tree/3.4-reset-profiler |
|
Some comments posted in this last commit: @derrabus If OK, I invite you to cherry-pick it and squash it with yours. Then it'll LGTM. |
|
@nicolas-grekas About your comment there:
Because the profiler is pretty much unusable, if we don't re-enable it.
I wouldn't see this feature for leaks only. The main issue with the profiler is that if the kernel handles multiple requests, the profiler partly still contains data from previous requests. Because of this, it displays inaccurate data. Slowly leaking memory is more or less a side effect of this and not such a big deal since the profiler is supposed to be disabled on prod anyway.
Not so many, actually. The Symfony contributors did a pretty good job on creating stateless non-leaking services. 😎 But yes, there are a few services that carry request-related state that needs to be reset (see #24291).
Actually, we're the ones disabling the profiler, so we don't profile the profiler and render a debug toolbar inside, you know, the debug toolbar. 😉 So, if you revert the re-enabling of the profiler (as you did in your commit), the effect is that you get a debug toolbar on the first request, but not on any subsequent ones handled by the same kernel.
To me, reset is meant to be used to purge any (master-)request-related state from services that otherwise would alter the processing on subsequent requests handled by the same kernel. |
Oh cool, that's the piece I was missing :) Instead of requiring several calls, which is still something I think we should avoid, I propose to add an "$enable" argument to the constructor of |
|
@derrabus here is what I have in mind: |
|
Yes, that's what I mentioned in #24289 (comment) already:
But that's merely a matter of taste, I guess. If it's more important to you to not allow multiple reset methods, we can go that way. I'm going to merge your changes now. |
d5f0646 to
0bc33fc
Compare
|
But tests are failing for now :) |
|
Yeah, you broke them. 😜 nicolas-grekas@10c1691#diff-3c1b20ab38677b0407a30d2c83278e66R60 I'm on it. 😃 |
0bc33fc to
76fb1bd
Compare
76fb1bd to
8c39bf7
Compare
|
Tests are green again. The failure on Travis should be resolved when merging the PR. |
|
Thank you @derrabus. |
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.
|
PR welcomed on master to remove the legacy code & update the interfaces. |
|
I've already started to work on it. |
…abbuh) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] implement reset() in DumpDataCollector | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24289 | License | MIT | Doc PR | Commits ------- cd602d6 implement reset() in DumpDataCollector
This PR adds the ability to reset the profiler between requests. Furthermore, the profiler service has been tagged with the new
kernel.resettag 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.