Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Optimize PHP function calls #196

Merged
merged 2 commits into from
Aug 1, 2017

Conversation

smuggli
Copy link

@smuggli smuggli commented Jul 4, 2017

Those optimizations target improving OPCODES.

@smuggli
Copy link
Author

smuggli commented Jul 4, 2017

Here are two showcases which demonstrate the optimazations.

is_string:
https://3v4l.org/sSjcd/vld#output
https://3v4l.org/Ufg8r/vld#output

array_keys:
https://3v4l.org/S8Bse/vld#output
https://3v4l.org/qRcHaJ/vld#output

I ran the PHPBench scripts but I am not sure if the report is necessary for that.

Also such optimazations already have been made:
https://github.com/zendframework/zend-servicemanager/blob/master/src/ServiceManager.php#L395
https://github.com/zendframework/zend-servicemanager/blob/master/src/ServiceManager.php#L396

@snapshotpl
Copy link
Contributor

Run PHPBench scripts to show performance improvement

@smuggli
Copy link
Author

smuggli commented Jul 4, 2017

The reports overall vary a bit, some are better and some worse.
before.txt
optimized.txt

@Ocramius
Copy link
Member

Ocramius commented Jul 4, 2017

Could you replace those isset() checks with ?? overall? Also, variadics may help too in private API

@smuggli
Copy link
Author

smuggli commented Jul 5, 2017

@Ocramius Yes, I can replace them but this implies that support for PHP < 7 would be dropped. Has this been decided yet? For variadics I will see what I can do. Thanks for the comment.

@MichaelGooden
Copy link

@smuggli The plan moving forward is to start pinning major components against PHP 7.1.

Source: https://framework.zend.com/blog/2017-06-06-zf-php-7-1.html

@stefanotorresi
Copy link
Contributor

stefanotorresi commented Jul 5, 2017

Honestly, I don't find the performance argument very compelling.
This is a change that would need to be propagated on every ZF component for consistency, and it would also be appropriate to put a coding standards rule in place.
Is it all truly worth it?

@Ocramius
Copy link
Member

Ocramius commented Jul 5, 2017 via email

@smuggli
Copy link
Author

smuggli commented Jul 5, 2017

@MichaelGooden Thanks completely missed that.

@stefanotorresi It is not a big gain but still something. For now I just started with ServiceManager to see if changes like that are welcome ;). Haven't made any thoughts on a coding standard rule yet.

@Ocramius Just replaced the issets with ?? for now because I am running out of time for today.

@michalbundyra
Copy link
Member

@smuggli This PR should target develop, not master.

@smuggli smuggli changed the base branch from master to develop July 5, 2017 13:00
@smuggli
Copy link
Author

smuggli commented Jul 5, 2017

@webimpress Changed it to develop. Thanks!

@michalbundyra
Copy link
Member

@smuggli I've created separate PR to drop PHP 5.6 and 7.0 support - #197. I will work on PHPCS sniff (in zend-coding-standard library) to detect function calls without \. It shouldn't be complicated.

@Ocramius
Copy link
Member

Ocramius commented Jul 5, 2017

commit smuggli/zend-servicemanager@fc81dee is unrelated, and the patch needs to be rebased to get rid of it

@smuggli smuggli force-pushed the optimize_opcodes branch 2 times, most recently from 775c5a1 to 2ae8b16 Compare July 6, 2017 09:11
@smuggli smuggli force-pushed the optimize_opcodes branch from 2ae8b16 to 32e8b82 Compare July 6, 2017 09:19
@kynx
Copy link
Contributor

kynx commented Jul 6, 2017

Would use function array_key_exists achieve the same thing? It would avoid 100s of lines of code changes and be much easier on the eyes ;) Though maybe harder to sniff.

@MichaelGooden
Copy link

@kynx It does appear to achieve the same outcome:

With use function statement: https://3v4l.org/QeVpj/vld#output
Without: https://3v4l.org/sSjcd/vld#output

@Ocramius
Copy link
Member

Ocramius commented Jul 7, 2017

@kynx nice one!

@kynx
Copy link
Contributor

kynx commented Jul 7, 2017

Cheers @Ocramius !

On a side note I just found out PHPStorm can auto-import from global scope. Nice!

@michalbundyra
Copy link
Member

Here is my sniff to import all used internal functions:
zendframework/zend-coding-standard@19e353b

I've created PR #198 after applying the sniff on develop branch.

Should we do the same with constants?
https://3v4l.org/Hlh9d/vld#output
https://3v4l.org/uSN1i/vld#output

@Ocramius
Copy link
Member

Ocramius commented Jul 9, 2017

Should we do the same with constants?

I don't think that we use many internal constants except for:

  • exceptions
  • filesystem-related operations
  • array_map operations

All of the above are usually improved by reducing the operation amount itself, rather than these micro-optimisations...

@smuggli
Copy link
Author

smuggli commented Aug 1, 2017

@Ocramius Sorry had no time working on that. To keep it simple, I would not do any variadics changes yet. Anything else what should be done before merging?

@Ocramius Ocramius added this to the 3.4.0 milestone Aug 1, 2017
@Ocramius Ocramius self-assigned this Aug 1, 2017
@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017

@smuggli this is good to go as-is, thanks!

@Ocramius Ocramius merged commit 1afca73 into zendframework:develop Aug 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants