Which is better in PHP: suppress warnings with '@' or run extra checks with isset()?

Question

For example, if I implement some simple object caching, which method is faster?

1. return isset($cache[$cls]) ? $cache[$cls] : $cache[$cls] = new $cls;

2. return @$cache[$cls] ?: $cache[$cls] = new $cls;

I read somewhere @ takes significant time to execute (and I wonder why), especially when warnings/notices are actually being issued and suppressed. isset() on the other hand means an extra hash lookup. So which is better and why?

I do want to keep E_NOTICE on globally, both on dev and production servers.

Tuberculin

Those two blocks of code don't do the same thing. – Heartwood

@deizel it's a function parameter so it is guaranteed to be defined in this context - forgot to mention that – Tuberculin

What if $cls is not a string/integer? :) – Upi

The equivalent would be return !empty($cache[$cls]) ? $cache[$cls] : $cache[$cls] = new $cls; – Heartwood

Why don't you test it yourself? – Wendall

Wouldn't version 2 hide any error if $cls was not a valid class name? – Vashti

@Vashti to my understanding @ suppresses only the subexpression next to it, so it won't hide errors arising with new $cls if that's what you mean. – Tuberculin

Essentially what I was getting at was that the second line may not do what you want it to. A cached falsey value such as false, "" or 0 will result in overriding the cache each time it's called. I recommend reading the type comparison table. – Heartwood

To answer my comment, you would end up suppressing the following: "Warning: Illegal offset type in ...". Happy debugging. – Upi

@Heartwood as you can see from my code, the cached value is always an object and $cls is always a string (otherwise new $cls would fail) – Tuberculin

@mojuba, while that is true in the context of your code, it does not hold for the general case, which is why I was bringing attention to it. It would do no good for someone to look at this question and assume that they can minify all their caching code with this pattern. – Heartwood

The performance of the @ is being improved with each new PHP version, but that's all I know. The rest is coding style I'd say, this was never a performance issue first-hand. It's how you deal with errors and warnings. – Hardunn

Answer 1

I ran timing tests for both cases, using hash keys of various lengths, also using various hit/miss ratios for the hash table, plus with and without E_NOTICE.

The results were: with error_reporting(E_ALL) the isset() variant was faster than the @ by some 20-30%. Platform used: command line PHP 5.4.7 on OS X 10.8.

However, with error_reporting(E_ALL & ~E_NOTICE) the difference was within 1-2% for short hash keys, and up 10% for longer ones (16 chars).

Note that the first variant executes 2 hash table lookups, whereas the variant with @ does only one lookup.

Thus, @ is inferior in all scenarios and I wonder if there are any plans to optimize it.

Tuberculin

I can't see them trying too hard to optimize this. It's a feature thats generally meant as a last resort when all other attempts to solve an error are in-effective. – Sclerodermatous

@Ben Griffiths what I like about programming (among other things) is the possibility of finding new creative ways of using the instrumentation we are given. For example, exceptions can sometimes be used for things other than passing error conditions down the call stack. Similarly, @ could have become part of the language, just like exceptions etc, if the implementation wasn't so awkward. Anyway, it's clearly not an option in my case at least today. – Tuberculin

True, but I think in this case there's already a comprehensive exception setup for people to play with, hence this being a last resort tool really. – Sclerodermatous

Answer 2

I wouldn't worry about which method is FASTER. That is a micro-optimization. I would worry more about which is more readable code and better coding practice.

I would certainly prefer your first option over the second, as your intent is much clearer. Also, best to keep away edge condition problems by always explicitly testing variables to make sure you are getting what you are expecting to get. For example, what if the class stored in $cache[$cls] is not of type $cls?

Personally, if I typically would not expect the index on $cache to be unset, then I would also put error handling in there rather than using ternary operations. If I could reasonably expect that that index would be unset on a regular basis, then I would make class $cls behave as a singleton and have your code be something like

return $cls::get_instance();

Enrage

Thousands of lines of code isn't a lot - millions is. Such tiny speed differences shouldn't impact you unless you have insane levels of traffic... – Sclerodermatous

@Omar Jackman the second variant is shorter, cleaner and more expressive. I'm only worrying about its speed to be honest. – Tuberculin

@Tuberculin the part that seams unreadable is the "@$cache[$cls] ?:" which tells me your not going for readability. It shows me that you are trying to shorten your code as much as possible because you'r hard drive only has about 2K of free space. – Gautier

@Omar Jackman I'm trying to shorten my code to make one hash lookup instead of 2. In some cases that may give you a significant performance gain. – Tuberculin

@Omar Jackman plus a ?: b is idiomatic and is as readable (perhaps even more so) as a ? b : c – Tuberculin

It might be readable at that level, but it's horrible for nesting. If you don't nest with these and use regular if/else than you have introduced two styles of coding in one application. How many people maintain this code (I imagine A LOT being that you need such high performance it'll be a large app). Multi coding styles in one app can introduce more problems down the line... – Sclerodermatous

This is not a micro-optimization once this runs repeatedly at a high-enough number, e.g. the context of a loop or recursion. – Loanloanda

Answer 3

The isset() approach is better. It is code that explicitly states the index may be undefined. Suppressing the error is sloppy coding.

According to this article 10 Performance Tips to Speed Up PHP, warnings take additional execution time and also claims the @ operator is "expensive."

Cleaning up warnings and errors beforehand can also keep you from using @ error suppression, which is expensive.

Additionally, the @ will not suppress the errors with respect to custom error handlers:

http://www.php.net/manual/en/language.operators.errorcontrol.php

If you have set a custom error handler function with set_error_handler() then it will still get called, but this custom error handler can (and should) call error_reporting() which will return 0 when the call that triggered the error was preceded by an @.

If the track_errors feature is enabled, any error message generated by the expression will be saved in the variable $php_errormsg. This variable will be overwritten on each error, so check early if you want to use it.

Anticipative

I'm curious why is it "sloppy coding", and how do you define sloppy coding in general? – Tuberculin

I consider it sloppy coding because it is clear the language is trying to warn about a potential problem, but the code wants to suppress the warning instead prevent it. In general, I would define sloppy coding as using undeclared/uninitiated variables, using deprecated features knowingly, letting the logs fill with "harmless" warnings/notices, no code comments, etc. – Anticipative

Isn't it also clear that the language wants you to be able to suppress errors, hence the existence of the error control operator? Having undeclared/uninitiated variables is very common when dealing with user input. And code is so much shorter and readable when I can write things like this: $value = @$_GET['value'] ? $_GET['value'] : $default_value; Imagine having a dozen or so of these. That's just a really easy way to assign default values. What's the downside? – Grafton

Answer 4

@ temporarily changes the error_reporting state, that's why it is said to take time.

If you expect a certain value, the first thing to do to validate it, is to check that it is defined. If you have notices, it's probably because you're missing something. Using isset() is, in my opinion, a good practice.

Jepson

Answer 5

I think you have your priorities a little mixed up here.

First of all, if you want to get a real world test of which is faster - load test them. As stated though suppressing will probably be slower.

The problem here is if you have performance issues with regular code, you should be upgrading your hardware, or optimize the grand logic of your code rather than preventing proper execution and error checking.

Suppressing errors to steal the tiniest fraction of a speed gain won't do you any favours in the long run. Especially if you think that this error may keep happening time and time again, and cause your app to run more slowly than if the error was caught and fixed.

Sclerodermatous