Cleanup method signatures.

[dereuromark]

As per guidelines only public methods should be typehinted as it is not cost-free.

This mainly removes the typehinting of protected methods that are mainly used from within the class.
Protected methods in often extended classes like Exception are not touched.

[jippi]

How does that affect performance? I dislike this change a lot..

[ADmad]

Here's what SO has to say about it.

[markstory]

Why not use new style arrays?

[ADmad]

Tests failing.

[jippi]

I really don't like this... it's a nice defense against users doing wrong things.. and yes, users do mess around in the internals all the time, extending, replacing classes..

Considering we do this for Cake 2.x as well, and the bownty app still manages to service 95th percentile in less than 40ms I really really don't think it's a worthwhile "optimization" - we should optimize for reading the code and making it clear to understand than to the CPU in these cases.

If you really need this kind of micro optimization, you can probably afford one more web box, it's an extreme edge case performance wise, even for a site like bownty with 100k+ requests a day that are stupidly complex.

It's a lot more simple and fast to read the core code when you don't have to jump back and forth between the method signature and a huge doc block to deduce the basics like object types.

The SO article do not provide any kind of useful benchmarks to backup their claim either, it's pure guesswork that "yes, it's slower" - which could just be a few ms which imo makes keeping the signatures a no brainer

[AD7six]

What Jippi said.

If it has a measurable performance cost (I find that unlikely, a SO post from 4 years ago doesn't really provide much motivation to consider it either) it would be more appropriate to (as a user) remove the type hints - which wouldn't be a particularly difficult sed call.

[dereuromark]

Note that in the past some added typehints for protected methods (which was actually also my preferred way) and that was also declined or disliked.
So we should try to find a consistent way of handling this then IMO.

[dereuromark]

And you will also have to remove the statement here: http://book.cakephp.org/3.0/en/contributing/cakephp-coding-conventions.html#typehinting

We only typehint public methods, though, as typehinting is not cost-free

[dereuromark]

@ADmad

Argument 1 passed to iterator_to_array() must implement interface Traversable, null given

I was a little bit perplexed about how to fix that one @ https://travis-ci.org/cakephp/cakephp/jobs/28184081

[markstory]

I can look into the performance impact of typehinting later this week if people are interested.

[ADmad]

I didn't mean that SO link to be an authoritative guide just a reference :) The stats provided by @jippi make lot more sense.

@dereuromark I don't have any idea either.

[Marlinc]

To be honest, I don't like this change either because you will need to look at the docblock in order to see the type.

[dereuromark]

@Marlinc In that case we should not promote the opposite (= no typehinting for protected methods). Instead we should promote defensive coding and also typehint protected methods consistently throughout the core.

[markstory]

I don't understand the complaints about having to look at the doc blocks. The doc block is generally 1-4 lines higher than the method signature, is this a big issue?

[Marlinc]

Well it might not be such a big problem as I think it is because most IDE's read the doc blocks very well.

[bcrowe]

Having a type hint vs. having to read the docblock indeed isn't really a huge win. But, considering the argument of performance: Realistically, if your application is getting enough traffic that this would ever become an issue... one should look into migrating from shared-hosting/a single node, etc. This kind of micro-optimization seems to only cater to an application that's pushing its shared-hosting / VPS's limits, but for some reason cannot afford to scale to additional nodes or a more powerful shared-hosting plan, etc? Personally, I'd much rather see defensive code. I think @dereuromark's last comment on promoting the opposite would be more suitable.

[jippi]

👍 for defensive code rather than micro optimizations like these :) I'm all for is_null vs === NULL since they don't really change anything in terms of readability or code execution.

This change does both, and I'm not in favor of either :)

[dereuromark]

With the changes in cakephp/docs#1468 we can probably close this as invalid change. We now encourage typehinting in general then for a more defensive approach.

We still didn't investigate the performance impact yet, though.
This would be interesting if we add typehints to all protected methods now, as well.

[markstory]

I'm planning on looking at typehint impacts one of these days 😄

[lorenzo]

@markstory take a look at this http://micro-optimization.com/type-hinting-slow-down

[markstory]

67% is a non-trivial bump. I think sticking with only putting typehints on public methods and key protected methods makes sense. Adding typehints to every method seems like overkill to me.

[dereuromark]

So which of the above changes would you find worth reverting to the way it was before then?
Most of those seem to be internal ones (non key so to speak).

[jippi]

@lorenzo that test is from 2011, can you re-run it on a modern php version with opcache ? :)

update

-> time php bench.php
1.541451215744  1.228688955307

real    0m2.884s
user    0m2.855s
sys 0m0.019s

[lorenzo]

I did, got similar results. 30% worse in 5.5

[lorenzo]

Not sure if that would be different in 5.4 or with a different processor...

[markstory]

30% worse is significant enough to warrant not using typehints on every protected method.

[milesj]

That benchmark is doing a million loops. Find me an app that ever does a million loops and I'll show you a shitty implementation. This change is a terrible one and the optimization improvements would be negligible in the real world.

[markstory]

I agree that no single request should be calling a million methods. But it is not unreasonable for an application ofdecent complexity to call a few thousand methods, which will add up. I admit this could become a slippery slope where all sorts of silly micro-optimizations can be justified. Personally, I don't see a huge amount of value in typehints as they don't halt execution, and php is not statically typed no matter how many hints you use.

[milesj]

Type hints throw fatal errors, which do halt execution. Not sure what you're trying to say. You're sacrificing type integrity/safety for a few micro-optimizations.

[markstory]

My bad, you're right. I thought they just emitted warnings for some silly reason. I'm not against type hints on public methods, or even commonly extended protected methods. I don't think there is much value in putting type-hints on every method.

[dereuromark]

Let's just close this then. Before someone goes all kamikaze on us ;) It ain't worth it.

[markstory]

Ok.

[ravage84]

Even though it has already been decided:

For me, a framework should tend to go a more stable, may be even defensive, way.
Even on non-public methods as they could be used by extension developers in some way.

If someone really would need the extra speed and would be willing to sacrifice the benefit of type hinting, he could craft a patch or a script, which removes the type hints from the core.

Or if we really identify one or two non-public methods, which are used really very often and thus make a great impact, we still could decide to cherry pick these to be removed.

At last: Removing security for speed within a framework is easier for a end user developer than the opposite.