[TwigBundle] Created stopwatch tag for profiling templates #7953

wouterj · 2013-05-06T19:17:21Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#2630

This PR adds a new tag to Twig which you can use to time parts of a template and see it in the timing tab of the profiler.

Usage:

{% stopwatch foo %}
... some things that gets timed
{% endstopwatch %}

This feature is based on the idea of @lsmith77 (see also our twitter conversation: https://twitter.com/lsmith/status/330772561413672962 )

lsmith77 · 2013-05-06T19:21:29Z

thank you for this .. works perfectly.
note we discussed at length if this should go into the TwigBundle or into WebProfilerBundle. in the end we agreed on putting it into TwigBundle since this way one can even put template using this tag into production where the profiler is usually disabled.

lsmith77 · 2013-05-06T19:23:07Z

src/Symfony/Bundle/TwigBundle/Node/StopwatchNode.php

+    {
+        $name = $this->getAttribute('name');
+
+        $compiler


we might want to pass a parameter into the StopwatchTokenParser and from there into StopwatchNode if the stopwatch service is even available. If so we would then not even write the startEvent/stopEvent code into the cached PHP.

I thought about that too. I'll implement something like this

fabpot · 2013-05-06T20:21:13Z

I would have put the code into the Twig bridge instead as this is where the integration of Twig and components happens. That would make the tag easily available for Drupal and Silex for instance.

lsmith77 · 2013-05-06T20:22:22Z

ah doh .. makes sense.

lsmith77 · 2013-05-06T20:48:38Z

src/Symfony/Bundle/TwigBundle/Extension/StopwatchExtension.php

+             * Some stuff which will be recorded on the timeline
+             * {% endstopwatch %}
+             */
+            new StopwatchTokenParser(),


you are missing the $stopwatchIsAvailable parameter here

wouterj · 2013-05-07T05:55:05Z

@fabpot I've moved the code to the twig bridge

stloyd · 2013-05-07T06:31:03Z

src/Symfony/Bridge/Twig/Extension/StopwatchExtension.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\TwigBridge\Extension;


Namespace is not correct, should be: Symfony\Bridge\Twig\Extension.

mpdude · 2013-05-07T23:22:20Z

Great! That deserves a "living on the edge" blog entry :-)

linniksa · 2013-05-07T23:54:33Z

src/Symfony/Bridge/Twig/TokenParser/StopwatchTokenParser.php

+        $body = $this->parser->subparse(array($this, 'decideStopwatchEnd'), true);
+        $stream->expect(\Twig_Token::BLOCK_END_TYPE);
+
+        return new StopwatchNode($name, $body, $this->stopwatchIsAvailable, $lineno, $this->getTag());


we can not simply return the $body if not stopwatchIsAvailable?

I need to test that. If it works, it's definitelly better than what we now have.

tested and it works, it's implemented now

fabpot · 2013-05-09T13:05:00Z

You should add a note in the CHANGELOG of the Twig bridge about this addition.

Tobion · 2013-05-09T13:20:54Z

What happens when u have multiple stopwatch blocks with the same name? Does it get aggregated or overwritten?

{% stopwatch foo %}
... some things that gets timed
{% endstopwatch %}

uninteresting stuff

{% stopwatch foo %}
... some things that gets timed
{% endstopwatch %}

stof · 2013-05-09T14:01:10Z

Tests are missing

stof · 2013-05-09T14:03:35Z

btw, using NAME_TYPE forbids having dots in the event name. Shouldn't we allow using a string too ?

And would it make sense to allow passing any Twig expression instead of only compile-time values ? Nothing in the current code requires to be able to use the event name at compile-time so we could make it dynamic.

wouterj · 2013-05-09T14:29:54Z

@stof should I add a StopwatchTokenParserTest? And what exactly do you mean with your last paragraph? Allowing string is a great idea, I'll add it.

@Tobion due to the way the stopwatch works, only the first one is recorded. I'll throw an error if the name already exists.

Todo

Add test
Allow strings
Throw error when name already exists
Make it dynamic (?)

fabpot · 2013-05-09T15:12:52Z

We should use start, stop instead to allow what @Tobion describe. Adn we should also support categories.

wouterj · 2013-05-09T16:19:28Z

@fabpot I can understand it wrong, but I already use start and stop?

stof · 2013-05-09T22:34:16Z

@wouterj here is what I mean:

{% stopwatch 'foo' %}
    {# equivalent of current stopwatch foo #}
{% endstopwatch %}

{% set bar = 'foo' %}
{% stopwatch bar %}
    {# uses the variable #}
{% endstopwatch %}

{% stopwatch (bar ~ 'test')|upper %}
    {# uses a complex expression #}
{% endstopwatch %}

stof · 2013-05-09T22:35:51Z

hmm, it could cause an issue to stop as we would need to stop the one we started, not to re-evaluate the expression as variables could have changed inside the body

wouterj · 2013-05-09T22:45:02Z

Thanks for the explaination! But do you really think it's worth it? The core block tag doesn't support it and I don't think people will use such a things for debugging.

stof · 2013-05-09T22:55:04Z

@wouterj block names have to be known at compile time. So it is a bad comparison

matthiasnoback · 2013-05-10T15:23:40Z

I think it would be good to support {% endstopwatch 'name' %} to make sure the right stopwatch is stopped. As far as I can see, it is also not possible to nest these tags, am I right?

stof · 2013-05-10T15:31:17Z

@matthiasnoback The end tag should stop the corresponding opening tag. So passing the name explicitly is only a way to allow mistakes as it must be the same value.

And nothing forbids you to nest them if you use different names

wouterj · 2013-05-10T17:21:06Z

I've implemented everything that is said here, except for 2 things:

Dynamic names. I fail to see when this is used.
Fabien's last comment (I can't follow that one). It throws an exception now if 2 names are equal

wouterj · 2013-07-08T16:18:36Z

ping @fabpot

stloyd · 2013-07-08T16:37:17Z

src/Symfony/Bridge/Twig/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 2.3.0
 -----

+ * added stopwatch tag to time templates with the WebProfilerBundle


This is not correct, 2.3 is already released.

fabpot · 2013-08-11T20:59:16Z

Closing in favor of #8719

This PR was merged into the master branch. Discussion ---------- [TwigBundle] Created stopwatch tag for profiling templates | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7953 | License | MIT | Doc PR | symfony/symfony-docs#2630 This PR is the continuation of #7953 This PR adds a new tag to Twig which you can use to time parts of a template and see it in the timing tab of the profiler. Usage: ````jinja {% stopwatch foo %} ... some things that gets timed {% endstopwatch %} ```` Commits ------- 29a58e7 change the stopwatch argument to be any valid expression 4590974 removed code that prevents the stopwatch to work properly 2f67776 removed unneeded safeguard as it's already done during compilation bbad387 fixed CS f39ed57 Created stopwatch tag

lsmith77 reviewed May 6, 2013
View reviewed changes

stloyd reviewed May 7, 2013
View reviewed changes

linniksa reviewed May 7, 2013
View reviewed changes

wouterj mentioned this pull request May 10, 2013

[WCM] Added stopwatch tag symfony/symfony-docs#2630

Merged

stloyd reviewed Jul 8, 2013
View reviewed changes

Created stopwatch tag

46aa97c

fabpot mentioned this pull request Aug 11, 2013

[TwigBundle] Created stopwatch tag for profiling templates #8719

Merged

fabpot closed this Aug 11, 2013

wouterj deleted the twig_stopwatch_helper branch May 1, 2014 07:55

Uh oh!

[TwigBundle] Created stopwatch tag for profiling templates #7953

[TwigBundle] Created stopwatch tag for profiling templates #7953

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Todo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!