search

lstrojny / functional-php

Primitives for functional programming in PHP
MIT License
1.98k stars 204 forks source link

refactor: Add leading \ before function invocation to speed up resolving #176

Closed damianopetrungaro closed 5 years ago

lstrojny commented 6 years ago

See https://github.com/lstrojny/functional-php/pull/171#issuecomment-433900318

damianopetrungaro commented 6 years ago

@lstrojny https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/ This is a good benchmark about this optimization.

jdreesen commented 6 years ago

Afaik the performance benefit is negligible for most functions because it's very minimal and only accounts for the first run, because PHP's OpCache remembers for subsequent calls whether the function is global or local.

There are, however, some functions that profit from prefixing them with \, because the PHP compiler optimizes them internally (which is only possible with the prefix). These functions are listed here: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php#L339-L378

lstrojny commented 6 years ago

@damianopetrungaro I wonder if we just want to run php-cs-fixer with the relevant fixers over the codebase as a default. CI integration could work in a way that the build fails if the fixer finds anything. Would you be interested in working on that?

phanan commented 6 years ago

@lstrojny May I suggest StyleCI?

damianopetrungaro commented 6 years ago

Yeah it's fine, this is how I added them actually. I suggest Travis because we already have it in place.

phanan commented 6 years ago

The thing about StyleCI is it can fix the errors for you, and it takes literally 1-2 minutes to set up. I've been using both Travis and StyleCI in several of my projects without any complaints.

damianopetrungaro commented 6 years ago

Actually, I would avoid having a fix by a CI, just sayin' 😂

phanan commented 6 years ago

@damianopetrungaro By default, StyleCI sends a PR containing the fixes. You can optionally configure it to have those PR's merged directly (which I do, because it's too good). I believe Laravel has the same setup.

damianopetrungaro commented 6 years ago

@lstrojny 🏓

lstrojny commented 5 years ago

LGTM, can you add php-cs-fixer it to the travis.yml and make it fail if there are changes?

damianopetrungaro commented 5 years ago

@lstrojny it's already doing it :) If the file won't be properly formatted the line: vendor/bin/php-cs-fixer fix --dry-run --diff --config=.php_cs.dist will fail 😄

lstrojny commented 5 years ago

OK, last remaining issue is the build with 7.3 and up. Can you check?

damianopetrungaro commented 5 years ago

@lstrojny is the PHP version of the 7.3 that looks bad.

    - friendsofphp/php-cs-fixer v2.13.1 requires php ^5.6 || >=7.0 <7.3 -> your PHP version (7.3.0RC5) does not satisfy that requirement.

I can take a look next week, I am quite busy atm. I'll mark it as unread so I'll look at it ;)

alexeyshockov commented 5 years ago

PHP-CS-Fixer changed their PHP requirements in v2.14.0 that has been released only on 4th of January 2019. Previous version was not officially compatible with 7.3.

So it should be OK as it, right now (with the new version), just rerun the build.

lstrojny commented 5 years ago

Merging, thank you very much!