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

Cache resolved asserters #569

Merged
merged 1 commit into from Feb 2, 2016

Conversation

Projects
None yet
8 participants
@jubianchi
Member

jubianchi commented Feb 2, 2016

This tiny patch let us improve performances on atoum tests (this is really clear with inline engine).

All the metrics above were generated using the phpunit-extension and the PHP-DI test suite (vendor/bin/atoum -d tests/UnitTest/ tests/IntegrationTest)

Before

PHP 7.0.0 (cli) (built: Dec  2 2015 13:06:23) ( NTS )
Copyright (c) 1997-2015 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2015 Zend Technologies
    with blackfire v1.9.0, https://blackfire.io, by Blackfireio Inc.

> Total tests duration: 29.46 seconds.
> Total tests memory usage: 4.00 Mb.
> Running duration: 29.53 seconds.

After

PHP 7.0.0 (cli) (built: Dec  2 2015 13:06:23) ( NTS )
Copyright (c) 1997-2015 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2015 Zend Technologies
    with blackfire v1.9.0, https://blackfire.io, by Blackfireio Inc.

> Total tests duration: 1.84 seconds.
> Total tests memory usage: 4.00 Mb.
> Running duration: 1.89 seconds.

Here is the diff generated with blackfire (blackfire run --samples=5 vendor/bin/atoum -d tests/UnitTest/ tests/IntegrationTest):

blackfire

jubianchi added a commit that referenced this pull request Feb 2, 2016

@jubianchi jubianchi merged commit 33728bc into atoum:master Feb 2, 2016

3 checks passed

Scrutinizer 1 updated code elements
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Hywan

This comment has been minimized.

Show comment
Hide comment
@Hywan

Hywan Feb 3, 2016

Member

👍
Impressive how it boosts performance :-/.

Member

Hywan commented Feb 3, 2016

👍
Impressive how it boosts performance :-/.

@Hywan

This comment has been minimized.

Show comment
Hide comment
@Hywan

Hywan Feb 3, 2016

Member

Good job!

Member

Hywan commented Feb 3, 2016

Good job!

@mikaelrandy

This comment has been minimized.

Show comment
Hide comment
@mikaelrandy

mikaelrandy Feb 3, 2016

Member

👍

Member

mikaelrandy commented Feb 3, 2016

👍

@Mandragora2

This comment has been minimized.

Show comment
Hide comment
@Mandragora2

Mandragora2 Feb 3, 2016

👍 ❤️

Mandragora2 commented Feb 3, 2016

👍 ❤️

@dupuchba

This comment has been minimized.

Show comment
Hide comment
@dupuchba

dupuchba commented Feb 3, 2016

👍

@@ -63,6 +64,10 @@ public function getNamespaces()
public function resolve($asserter)

This comment has been minimized.

@mageekguy

mageekguy Feb 3, 2016

Contributor

Alternative version (in my opinion, no impact on performance, but improve readability and more in the atoum's coding style):

    public function resolve($asserter)
    {
        if (! isset($this->resolved[$asserter]))
        {
            $class = null;

            if ($this->analyzer->isValidNamespace($asserter))
            {
                if (strpos($asserter, '\\') !== false)
                {
                    $class = $this->checkClass($asserter);
                }
                else foreach ($this->namespaces as $namespace)
                {
                    switch (true)
                    {
                        case $class = $this->checkClass($namespace . '\\' . $asserter):
                        case $class = $this->checkClass($namespace . '\\php' . ucfirst($asserter));
                            break 2;
                    }
                }
            }

            $this->resolved[$asserter] = $class;
        }

        return $this->resolved[$asserter];
    }
@mageekguy

mageekguy Feb 3, 2016

Contributor

Alternative version (in my opinion, no impact on performance, but improve readability and more in the atoum's coding style):

    public function resolve($asserter)
    {
        if (! isset($this->resolved[$asserter]))
        {
            $class = null;

            if ($this->analyzer->isValidNamespace($asserter))
            {
                if (strpos($asserter, '\\') !== false)
                {
                    $class = $this->checkClass($asserter);
                }
                else foreach ($this->namespaces as $namespace)
                {
                    switch (true)
                    {
                        case $class = $this->checkClass($namespace . '\\' . $asserter):
                        case $class = $this->checkClass($namespace . '\\php' . ucfirst($asserter));
                            break 2;
                    }
                }
            }

            $this->resolved[$asserter] = $class;
        }

        return $this->resolved[$asserter];
    }
@@ -15,6 +15,7 @@ class resolver
protected $baseClass = '';
protected $namespaces = array();
private $analyzer;
private $resolved = array();

This comment has been minimized.

@mageekguy

mageekguy Feb 3, 2016

Contributor

Maybe a static attribute is better here, to shared the cache between all instances of the resolver (even if there is only one instance in atoum, in theory).

@mageekguy

mageekguy Feb 3, 2016

Contributor

Maybe a static attribute is better here, to shared the cache between all instances of the resolver (even if there is only one instance in atoum, in theory).

This comment has been minimized.

@jubianchi

jubianchi Feb 3, 2016

Member

right!

@jubianchi

jubianchi Feb 3, 2016

Member

right!

@mageekguy

This comment has been minimized.

Show comment
Hide comment
@mageekguy

mageekguy Feb 3, 2016

Contributor

Very good job, the result is really impressive!

Contributor

mageekguy commented Feb 3, 2016

Very good job, the result is really impressive!

@SylvG

This comment has been minimized.

Show comment
Hide comment
@SylvG

SylvG commented Feb 3, 2016

👍

@mageekguy

This comment has been minimized.

Show comment
Hide comment
@mageekguy

mageekguy Feb 3, 2016

Contributor

And again, a huge warning: the impact of this patch will be less impressive with other engines (fork is main bottleneck with other engines, and with a fork, the cache is not so efficient because it will be recreated in each subprocess)!

Contributor

mageekguy commented Feb 3, 2016

And again, a huge warning: the impact of this patch will be less impressive with other engines (fork is main bottleneck with other engines, and with a fork, the cache is not so efficient because it will be recreated in each subprocess)!

@HitoTech

This comment has been minimized.

Show comment
Hide comment
@HitoTech

HitoTech commented Feb 3, 2016

👍

@jubianchi

This comment has been minimized.

Show comment
Hide comment
@jubianchi

jubianchi Feb 3, 2016

Member

@mageekguy totally right!

I'm already working on other performance improvements targeted at concurrent and isolate engines.

Member

jubianchi commented Feb 3, 2016

@mageekguy totally right!

I'm already working on other performance improvements targeted at concurrent and isolate engines.

@Hywan

This comment has been minimized.

Show comment
Hide comment
@Hywan

Hywan Feb 3, 2016

Member

@mageekguy, @jubianchi Maybe we can share the cache between process but it can really slow down the execution due to concurrent I/O. But inside the same test case (so the same process), it will speed up anyway as soon as we use an assertion more than once.

Member

Hywan commented Feb 3, 2016

@mageekguy, @jubianchi Maybe we can share the cache between process but it can really slow down the execution due to concurrent I/O. But inside the same test case (so the same process), it will speed up anyway as soon as we use an assertion more than once.

@mageekguy

This comment has been minimized.

Show comment
Hide comment
@mageekguy

mageekguy Feb 3, 2016

Contributor

@Hywan Cost of serialization/deserialization to transfert cache between each processus will cancel the cache benefit.

Contributor

mageekguy commented Feb 3, 2016

@Hywan Cost of serialization/deserialization to transfert cache between each processus will cancel the cache benefit.

@Hywan

This comment has been minimized.

Show comment
Hide comment
@Hywan

Hywan Feb 3, 2016

Member

@mageekguy Except if the cache is in a shared memory page: http://php.net/shmop.

Member

Hywan commented Feb 3, 2016

@mageekguy Except if the cache is in a shared memory page: http://php.net/shmop.

@mageekguy

This comment has been minimized.

Show comment
Hide comment
@mageekguy

mageekguy Feb 3, 2016

Contributor

@Hywan Shared memory page imply specific PHP extension which is rarely available "out of the box" on a workstation and it seems to me that it is unavailable on Windows. Moreover, this kind of solution (or any cache solution) imply lock management to avoid race condition. So, this solution add lot of complexity in the code in regards of a small impact on performance and a lack of portability. Not sure that wasting time to implements this kind of tradeoff is a good move, or only for the fun. And finally, you must serialize data to put them in a shared memory page… go back to the start of the story ;).

Contributor

mageekguy commented Feb 3, 2016

@Hywan Shared memory page imply specific PHP extension which is rarely available "out of the box" on a workstation and it seems to me that it is unavailable on Windows. Moreover, this kind of solution (or any cache solution) imply lock management to avoid race condition. So, this solution add lot of complexity in the code in regards of a small impact on performance and a lack of portability. Not sure that wasting time to implements this kind of tradeoff is a good move, or only for the fun. And finally, you must serialize data to put them in a shared memory page… go back to the start of the story ;).

@Hywan

This comment has been minimized.

Show comment
Hide comment
@Hywan

Hywan Feb 3, 2016

Member

@mageekguy It works on Windows :-).

Member

Hywan commented Feb 3, 2016

@mageekguy It works on Windows :-).

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