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

Added support for foo.some_thing === foo.someThing #1299

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

hason
Copy link
Contributor

@hason hason commented Dec 19, 2013

Alternative implementation of #934. Works well for methods, properties and array calls of objects with minimal performance impact. Results of twig benchmark (fabpot/twig-perf):

                               |       v1.12.3 |       v1.16.3 | origin/master | origin/methods | 
------------------------------------------------------------------------------------------------
empty                          |     13.5  0.0 |      8.8  0.0 |     14.4  0.0 |     20.5  0.0 | 
empty/B                        |      7.9  0.0 |     14.1  0.0 |     14.6  0.0 |     18.9  0.0 | 
simple_attribute               |     88.2  0.9 |     99.8  0.9 |     99.2  0.8 |     99.3  0.9 | 
simple_array_access            |    180.2  1.3 |    132.8  0.9 |    120.6  0.9 |    127.1  1.0 | 
simple_method_access           |     94.4  2.4 |     92.9  2.7 |    101.7  2.7 |    119.7  2.9 | 
simple_attribute_big_context/B |     90.5  0.9 |    101.3  0.9 |    100.5  1.0 |    116.1  1.3 | 
simple_variable                |    710.0  0.7 |    698.6  0.7 |    711.9  0.8 |    680.2  0.7 | 
simple_variable_big_context/B  |    702.2  1.3 |    668.3  1.3 |    795.9  1.2 |    674.3  1.2 | 
simple_foreach                 |     10.9  0.0 |     21.0  0.0 |     15.2  0.0 |     10.7  0.0 | 
simple_foreach/B               |     10.1  5.7 |     16.3  7.7 |      9.9  5.0 |     14.5  4.7 | 
empty_extends                  |      9.5  0.0 |     17.4  0.0 |     17.6  0.0 |     13.9  0.0 | 
empty_extends/B                |     16.1  0.0 |     17.5  0.0 |     10.2  0.0 |     22.6  0.0 | 
empty_include                  |     10.3  0.0 |     10.0  0.0 |     12.1  0.0 |      9.5  0.0 | 
empty_include/B                |     14.8  0.0 |     14.5  0.0 |     15.4  0.0 |     14.4  0.0 | 
standard                       |     22.1  0.0 |     12.6  0.0 |     12.9  0.0 |     14.0  0.0 | 
escaping                       |    101.8  1.2 |    110.9  1.2 |    107.5  0.8 |     99.2  1.0 |

@simensen
Copy link

Wow! This seems like it might be great. I don't know enough of the C code to know how well this is implemented. That is part of the reason I haven't had a chance to follow up on #934 yet. Thanks for running with this!

zend_property_info *pptr = (zend_property_info *) pDest;
APPLY_TSRMLS_FETCH();

if (!(pptr->flags & ZEND_ACC_PUBLIC) || (pptr->flags & ZEND_ACC_STATIC)) {
if ((pptr->flags & ZEND_ACC_PRIVATE) || (pptr->flags & ZEND_ACC_STATIC)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this change cause some issues with protected ? Note that I'm not sure about my comment as I'm not a C developer

@hason
Copy link
Contributor Author

hason commented Jan 21, 2015

@fabpot ping

This PR does not hurt performance #934 (comment) and works with properties and arrays #934 (comment)

composer.json Outdated
@@ -27,7 +27,8 @@
"forum": "https://groups.google.com/forum/#!forum/twig-users"
},
"require": {
"php": ">=5.2.4"
"php": ">=5.2.4",
"ext-pcre": "*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this one.

@fabpot
Copy link
Contributor

fabpot commented Jan 21, 2015

@tucksaun @jpauli Can you have a look at the C part of this PR?

if (is_object($object)) {
$class = get_class($object);
if (!isset(self::$cache[$class])) {
self::$cache[$class] = $this->getCacheForClass($class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the non-debug mode, what about keeping this cache on disk? Would that makes things faster? It looks like there is a lot going on in the getCacheForClass() method and I cannot believe that it does not have any impacts on real-world templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for the cache may be added in another PR.

@lsmith77
Copy link

can we give this PR some life again?

@elnur
Copy link

elnur commented Mar 9, 2017

😞

@fabpot fabpot closed this Mar 2, 2018
@fabpot
Copy link
Contributor

fabpot commented Mar 2, 2018

This PR was auto-closed because I've removed the master branch and this PR was based on master. I cannot reopen it unfortunately.

@fabpot fabpot changed the base branch from master to 2.x March 2, 2018 19:24
@fabpot
Copy link
Contributor

fabpot commented Mar 2, 2018

Changing the base branch to 2.x worked :)

@fabpot fabpot reopened this Mar 2, 2018
@fabpot
Copy link
Contributor

fabpot commented Feb 15, 2019

This one never moved forward as we did not get feedback on the C implementation. I propose to rebase it on 2.x where we don't have the C extension.

@hason
Copy link
Contributor Author

hason commented Jan 26, 2021

@fabpot The C extension in 1.x is no longer used. Should I update this PR on 1.x or 2.x?

@xabbuh
Copy link
Contributor

xabbuh commented Jan 26, 2021

@hason The C extension is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants