Cache results of `getTimezoneNameNoLocationSpecific` #30

Merged
merged 1 commit into from Jan 17, 2015

Projects

None yet

4 participants

@LukasReschke
Contributor

getTimezoneNameNoLocationSpecific is a very expensive operation and previously a simple looped time conversion (1000x) required more than two seconds to execute.

At ownCloud we use such date conversion actions pretty often and repeatedly and thus listing a folder with more than 20k files took more than 3 seconds longer just due to this single method.

A simple test script looping 10000 times shows the following performance gains:

screen shot 2015-01-17 at 23 08 15

(wasn't able to get the unit tests locally running – should pass on Travis though)

Ref owncloud/core#13434

@LukasReschke LukasReschke Cache results of `getTimezoneNameNoLocationSpecific`
`getTimezoneNameNoLocationSpecific` is a very expensive operation and previously a simple looped time conversion (1000x) required more than two seconds to execute.

At ownCloud we use such date conversion actions pretty often and repeatedly and thus listing a folder with more than 20k files took more than 3 seconds longer just due to this single method.
5b69f44
@coveralls

Coverage Status

Coverage increased (+0.01%) when pulling 5b69f44 on LukasReschke:use-simple-array-cache into 746398b on punic:master.

@mlocati
Member
mlocati commented Jan 17, 2015

Nice and useful approach! Thanks!

@mlocati mlocati merged commit 5b82f00 into punic:master Jan 17, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@LukasReschke LukasReschke deleted the LukasReschke:use-simple-array-cache branch Jan 17, 2015
@mlocati
Member
mlocati commented Jan 17, 2015

I'll create a new release in a few hours, to that you can reference to it in your composer.json.

@LukasReschke
Contributor

Give me half an hour to find some more low hanging fruits in the code – still not fully happy with the performance :)

@mlocati
Member
mlocati commented Jan 17, 2015

You can have all the time you want;) It's almost midnight here, and a wife and some night coding don't get along very well 😉

@Remo
Contributor
Remo commented Jan 18, 2015

@LukasReschke what was the problem of getting the tests working locally? In my case it was always enough to run composer update|install and phpunit. Would like to keep them as easy to use as possible..

@Remo
Contributor

have you tested what impact a simple string operation would have? I guess json_encode isn't expensive, but a string operation should still be faster. Not sure if it's worth the ugliness though..

@mlocati
Member
mlocati commented Jan 18, 2015

have you tested what impact a simple string operation would have? I guess json_encode isn't expensive, but a string operation should still be faster. Not sure if it's worth the ugliness though..

The problem here is that $value can also be a \DateTime, and its conversion to a string requires some checks/functions: I don't know if it would be much faster than that json_encode (and json_encode handles correctly \DateTimes).

@mlocati
Member
mlocati commented Jan 18, 2015

@LukasReschke what was the problem of getting the tests working locally? In my case it was always enough to run composer update|install and phpunit. Would like to keep them as easy to use as possible..

phpunit doesn't even require anything that's installed by composer. The only package installed by composer is apigen, that's used to build the API docs...

@Remo
Contributor
Remo commented Jan 18, 2015

You are of course right! The more I wonder what we did that the tests don't work.. From: Michele LocatiSent: Sunday, January 18, 2015 11:13To: punic/punicReply To: punic/punicCc: Remo LaubacherSubject: Re: [punic] Cache results of getTimezoneNameNoLocationSpecific (#30)
@LukasReschke what was the problem of getting the tests working locally? In my case it was always enough to run composer update|install and phpunit. Would like to keep them as easy to use as possible..

phpunit doesn't even require anything that's installed by composer. The only package installed by composer is apigen, that's used to build the API docs...

—Reply to this email directly or view it on GitHub.

@LukasReschke
Contributor

phpunit doesn't even require anything that's installed by composer. The only package installed by composer is apigen, that's used to build the API docs...

Mhm… For me without having the composer commands run and a lot of useless dependencies installed I get the following error:

PHP Warning:  require_once(/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php): failed to open stream: No such file or directory in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4
PHP Stack trace:
PHP   1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
PHP   2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
PHP   4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
PHP   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
PHP   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
PHP   7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
PHP   8. include_once() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

Warning: require_once(/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php): failed to open stream: No such file or directory in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4

Call Stack:
    0.0213     518976   1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
    0.0260     744728   2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
    0.0260     745352   3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
    0.0260     747968   4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
    0.0308    1124376   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
    0.0310    1132976   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
    0.0311    1133176   7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
    0.0312    1135880   8. include_once('/Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php') phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

PHP Fatal error:  require_once(): Failed opening required '/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php' (include_path='.:/usr/local/Cellar/php55/5.5.18/lib/php') in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4
PHP Stack trace:
PHP   1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
PHP   2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
PHP   4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
PHP   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
PHP   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
PHP   7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
PHP   8. include_once() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

Fatal error: require_once(): Failed opening required '/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php' (include_path='.:/usr/local/Cellar/php55/5.5.18/lib/php') in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4

Call Stack:
    0.0213     518976   1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
    0.0260     744728   2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
    0.0260     745352   3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
    0.0260     747968   4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
    0.0308    1124376   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
    0.0310    1132976   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
    0.0311    1133176   7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
    0.0312    1135880   8. include_once('/Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php') phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

This seems to be because your bootstrap.php includes the not existing composer autoloader: https://github.com/punic/punic/blob/b9b540d479489d2a35f409b926b28e215c65ca42/tests/bootstrap.php#L4

Either that should get removed (preferably if not required) or get documented in the project's README.

@Remo
Contributor
Remo commented Jan 18, 2015

And what happens if you run composer? From: Lukas ReschkeSent: Sunday, January 18, 2015 11:50To: punic/punicReply To: punic/punicCc: Remo LaubacherSubject: Re: [punic] Cache results of getTimezoneNameNoLocationSpecific (#30)
phpunit doesn't even require anything that's installed by composer. The only package installed by composer is apigen, that's used to build the API docs...

Mhm… For me without having the composer commands run and a lot of useless dependencies installed I get the following error:

PHP Warning: require_once(/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php): failed to open stream: No such file or directory in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4
PHP Stack trace:
PHP 1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
PHP 2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
PHP 3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
PHP 4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
PHP 5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
PHP 6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
PHP 7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
PHP 8. include_once() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

Warning: require_once(/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php): failed to open stream: No such file or directory in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4

Call Stack:
0.0213 518976 1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
0.0260 744728 2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
0.0260 745352 3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
0.0260 747968 4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
0.0308 1124376 5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
0.0310 1132976 6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
0.0311 1133176 7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
0.0312 1135880 8. include_once('/Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php') phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

PHP Fatal error: require_once(): Failed opening required '/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php' (include_path='.:/usr/local/Cellar/php55/5.5.18/lib/php') in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4
PHP Stack trace:
PHP 1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
PHP 2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
PHP 3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
PHP 4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
PHP 5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
PHP 6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
PHP 7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
PHP 8. include_once() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

Fatal error: require_once(): Failed opening required '/Users/lreschke/Programming/core/3rdparty/punic/punic/vendor/autoload.php' (include_path='.:/usr/local/Cellar/php55/5.5.18/lib/php') in /Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php on line 4

Call Stack:
0.0213 518976 1. {main}() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:0
0.0260 744728 2. PHPUnit_TextUI_Command::main() /usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar:605
0.0260 745352 3. PHPUnit_TextUI_Command->run() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:138
0.0260 747968 4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:148
0.0308 1124376 5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:651
0.0310 1132976 6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/TextUI/Command.php:817
0.0311 1133176 7. PHPUnit_Util_Fileloader::load() phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:77
0.0312 1135880 8. include_once('/Users/lreschke/Programming/core/3rdparty/punic/punic/tests/bootstrap.php') phar:///usr/local/Cellar/phpunit/4.3.4/libexec/phpunit-4.3.4.phar/phpunit/Util/Fileloader.php:93

This seems to be because your bootstrap.php includes the not existing composer autoloader: https://github.com/punic/punic/blob/b9b540d479489d2a35f409b926b28e215c65ca42/tests/bootstrap.php#L4

Either that should get removed (preferably if not required) or get documented in the project's README.

—Reply to this email directly or view it on GitHub.

@LukasReschke
Contributor

And what happens if you run composer?

Then it works and I get installed all apigen dependencies locally which is not really what I'd expect ;-)

@Remo
Contributor
Remo commented Jan 18, 2015

Got it, will look into this From: Lukas ReschkeSent: Sunday, January 18, 2015 11:55To: punic/punicReply To: punic/punicCc: Remo LaubacherSubject: Re: [punic] Cache results of getTimezoneNameNoLocationSpecific (#30)
And what happens if you run composer?

Then it works and I get installed all apigen dependencies locally which is not really what I'd expect ;-)

—Reply to this email directly or view it on GitHub.

@mlocati
Member
mlocati commented Jan 18, 2015

#37 if merged will allow run phpunit without composer. Shall we do that?

@Remo
Contributor
Remo commented Jan 18, 2015

I support this! Apigen is useful for us too, even if it's not useful when running the tests.. we can keep apigen and make the tests easier to run! From: Michele LocatiSent: Sunday, January 18, 2015 12:14To: punic/punicReply To: punic/punicCc: Remo LaubacherSubject: Re: [punic] Cache results of getTimezoneNameNoLocationSpecific (#30)#37 if merged will allow run phpunit without composer. Shall we do that?

—Reply to this email directly or view it on GitHub.

@mlocati
Member
mlocati commented Jan 18, 2015

Another solution would be to use composer install --no-dev.

@Remo
Contributor
Remo commented Jan 18, 2015

I find that strange, unit tests are closer to dev than prod. And at the end I support having less dependencies, especially if it's a small change like this From: Michele LocatiSent: Sunday, January 18, 2015 12:21To: punic/punicReply To: punic/punicCc: Remo LaubacherSubject: Re: [punic] Cache results of getTimezoneNameNoLocationSpecific (#30)Another solution would be to use composer install --no-dev.

—Reply to this email directly or view it on GitHub.

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