Should we use fully qualified function (FQN) calls like \is_array in the core?

Archive Discuss Topics
1

Discussion Topic

Using fully qualified function calls for PHP core functions can result in small performance gain (nanoseconds per call).
For example,

echo strlen('Hello World');
var_dump(null);
var_dump(false);
echo DIRECTORY_SEPARATOR;
return true;

// becomes:
echo \strlen('Hello World');
\var_dump(\null);
\var_dump(\false);
echo \DIRECTORY_SEPARATOR;
return \true;

Alternatively you can declare all PHP build in function used in the file like:
use function is_array;

For more details see:

Impact

I haven’t done a proper benchmark on TYPO3 project, but just to give a an estimation - we’re talking about gains around 0.1ms = 17k (PHP function calls on introduction package page rendering) * 0,006us gain.

Whole core should be migrated to new syntax. When backporting code to older branches slashes should be stripped.

Pro

  • little performance gain

Con

  • makes code harder to read
  • Symfony decided against introducing it
  • existing benchmarks are very low level (testing function calls) no TYPO3 benchmarks were done, so the gain for real use case is unknown
  • there is a workaround available - a composer plugin which automatically changes code in vendor, and other tools which converts the code automaticaly
  • it might break some testing code (where namespaces were used for mocking global functions)

Possible solutions

  • don’t care (if somebody prepare a patch with FQN’s it will be merged as is, we will get mixed styling in the core)
  • do not introduce FQN’s
  • introduce FQN’s with additional use statements
  • introudce FQN’s by prepending every call with slash

Organizational

Topic Initiator: Tymoteusz Motylewski
Topic Mentor: Tymoteusz Motylewski

2

Hey,

the last con especially is a pro in my book: If backslashing a function actually breaks it means that - though it was using the same name as the top level one - it was probably overwritten in a custom namespace which imho is pretty bad.

For me on the pro side:

  • semantically correct as these functions reside on the top level namespace
  • no chance of accidentally overwriting a PHP core function with own code

I actually don’t care about the performance impact, I’d agree that it is most likely insignificant - but for me it makes more semantical sense to use the qualifiers and produces cleaner and safer code.

For the other con arguments

  • Makes code harder to read: Yeah, heard that before everytime we change CGL :wink: spaces, braces, basically in every discussion again it’s “harder to read” as we are not used to it
  • Symfony decided against it - that’s no argument, and their argument in the threat is basically it’s not worth it performance wise - I agree there.

Thanks for opening the dicussion.

3

I was thinking about tests which are using namespaces to mock global functions - like mocking filesystem functions by overriding them in the namespace.
I have updated the description to reflect that.

safer code - maybe, but the the impact is really low here. What would be the real life example of the “exploit” or some unsafe code we’re protecting ourselves against?

For the semantical sense - I could agree on having additional “use” statement, to make it bold that this file is using global functions (PHPStorm has an option for that), but prepending every usage with slash is strange.

For the cleaner code, I strongly disagree here. Code cluttered with additional slashes everywhere ( “looks ugly” ;p ) is less readable and looks strange.
And as TYPO3 would probably be the first bigger player enforcing this rule, this coding style would be very unfamiliar to all developers.

4

This is just a matter of time. The introduction of namespaces was for me in the first place also very strange and hard to read. Never thought I would get familiar with it. But after a month or so it’s just normal and old not namespaced code looks strange.

I don’t see any really blocking con. Only thing would be if we would really be the only “bigger” project that uses it. So other devs are not familiar with it.

5

I would absolutely not do this! My list of cons:

  1. This should be (read: I completely expect it to be) solved by php internals in the future so namespaced vs. non-namespaced function calls become identical in performance (but opcaching maybe becomes marginally slower)
  2. It takes an incredible amount of time not just to explain but then to argue why we do this; to contributors when they create merge requests. If 1) then also happens, that’s a complete waste.
  3. The best increase in performance is, as far as I am aware, around a 4% increase in a quite theoretical setup. One reason why it didn’t pass into Symfony was that the error margin was higher than the measurable increase.
  4. If you want to increase performance in TYPO3, start with the way DB queries are being used. This is by far the ultimate resource drain - just as an example, generating a mega-menu in FE makes one query per page involved. Solve such problems and you’re looking at performance improvements in the 100’s of percents area.

In the end I absolutely expect that adopting this as standard, will do far more harm (in swallowing resources from core and contributors alike) than it could ever benefit (given a hypothetical 4% boost), and it is such low-hanging fruit for the PHP internals to solve that I’m sure they will solve it there.

In my eyes, this performance topic is comparable to, for example, spotting a difference in PHP functions where one function performs marginally better but requires a very odd syntax to use - in which case I’m sure we’d never jump in with both feet. Right?

6

Whilst actually supporting the intention to be specific in your code about whether something is local (to a namespace) or global, I dislike the syntax.

I don’t know any programming language where this is necessary or done in such a cryptic way. Remember we also have those useless $ prefix for variables in PHP, which is already a syntax mess. Adding now some backslahes on top, does not make the mess better.

Generally the usage of core functions is rather sparse. So it boils down to adding some 10 backslashes in an average class or so. This doesn’t bother me for readability.

What I’m strictly against is:

  • Prepending a backslash to system constants like true and false
  • Using the use ... construct do import global functions into the local scope.
7

do I read you correctly that you prefer having slashes over “use…” imports?
why?

8

I don’t have a strong opinion on that; I tend to agree that it should be avoided if there are no significant performance gains.

9

Because we use the same PHP function within a file mostly not more than one or two times. Hence, it’s a waste of space up there to have 10 lines for 10 different functions.
(and I do not consider PHP core functions as “dependencies” like other classes, which should be easily readable at the top of a file)

10

I prefer readability and pragmatism in contributing over an insignificant and rather theoretical gain in performance. As claus already pointed out, there are so many things in the core waiting where just a little Bit of Attention would lead to a Lot more Outcome Performance wise in a typical installation. So, with Limited manpower available, I suggest we Stick to those.

11

The main con for me is that backports need way more work and attention. Perhaps we also need to update all pending patches which is also quite a bit of work.
IMO it’s not worth the effort, but if enough people support it…

12

Backports and pending reviews were the first things that came to my mind as well. In total, I’d say the performance gain is not worth the trouble - as it adds quite a lot of code (= “characters”) to our code base.

In general I’m a huge favor of having a decision on these things:

  • Functions: is_array() vs. \is_array()
  • Our defined constants: \PATH_site vs. PATH_site
  • TYPO3-internal constants: \E_USER_DEPRECATED vs. E_USER_DEPRECATED
  • Importing global namespaces: new \RuntimeException() vs. use \RuntimeException; ... new RuntimeException()
13

To be honest, I am part of team “I don’t care”.

It seems to be correct code, PHPstorm suggested it always because of the performance gain (I know, it is a very small gain) and I was to lazy to adjust my settings. So I “fixed” it every time when I touched a piece of code (mostly the unit tests). But I see absolut no reason to “clean up” the whole core.

But reading the above statements feels a bit … strange. Readability is not valid point for me, as Susi said, we had (and will have) this argument with every CGL change. “Everything was better in the old days”. And backportability might be a valid point, if we would clean the whole core and need to backport a big punch of stuff all the time.

14

The readability really suffers. And as Claus pointed out, if this is really a performance issue it should be tackled by PHP itself.

An interesting experiment tough would be to add all those backslashes automatically. I guess this could be done by using the PHP tokenizer. But even if the result is that the performance gain is significant I would go for doing the added basckslashes automatically in a build process.

15

Your FYI, php_cs_fixer can add those backslashes. So it’s very little work to automate this.

16

I don’t like “all those slashes” because of readability - or even going for the use-statements. But I’m in the “don’t care” camp. I’m fine if we want to go that route. But “performance” shouldn’t be the major point driving us in that direction - but “because it’s cleaner” or maybe “because it’s PSR-something-standard” may be valid reasons :slight_smile:

17

This topic was automatically closed after 14 days. New replies are no longer allowed.