9 ms·

Improve PHP Disk I/O & remove Race conditions by using '@' error suppression

32 points by willmacdonald 14y ago

samarudge 14y ago
Maybe I didn't understand this article properly, but is it suggesting suppressing ALL filesystem related errors rather than handling them properly? OK I get that PHP doesn't support atomic operations, but there are so many different errors that could happen when working with files. Say a permissions error, if the permissions of the file were changed between it being created and deleted that's an issue that needs resolving before your tmp folder fills up, it's easy enough to fix and should be reported as an error (be it to the front-end or to a backend logging service). Surely there is a better way to handle these race conditions without just ignoring all filesystem related errors.
- RossM 14y ago
  No, I think the author knows the issues behind this muting - you just need to be smart about how you apply it. In the case of filemtime, it appears the only failure condition is a file not existing so we can safely ignore the E_WARNING it emits; it wouldn't be sensible to apply this everywhere (I hadn't thought about the permissions case myself). The best/real solution is probably implementing flock so that it can issue real filesystem locks, rather than relying on a file resource.
sandfox 14y ago
And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software. It's a broken solution to a problem that shouldn't really be there. Ignoring why the race condition is happening, what is wrong with something like Redis or heaven forbid semaphores/shared memory for locking. There are billions of ways of doing atomic updates right and this is not one of them --EDIT I hadn't even got as far as the disk i/o section, after reading no words can do justice to the complete lack of understanding shown here with regards to filesystems, caching, SSD's and linux.
- Gigablah 14y ago
  Not to mention you wouldn't even have a need for this so-called "optimization" in production, since you'd have your templates precompiled and cached (and turned the compile check off).
- mootothemax 14y ago
  And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software. It's a broken solution to a problem that shouldn't really be there. Very much agree with you. When I was first starting out programming commercially, one of the best bits of advice I received was something along the lines of "Take a step back and think about why you're using these crazy constructs - chances are there's a much simpler way of doing things." The lesson being: if you're having to write code like this, chances are it's because you're creating a mess. Take a step back and work out a simpler solution.
- xd 14y ago
  "And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software." Replace "PHP" with "Jewish" and would you find that comment of yours acceptable? Edit: I can only assume that the down voters find discrimination acceptable behavior in this community.
  - sandfox 14y ago
    Trollolol much? The religion of the author (is he even jewish? - I have no idea nor really care) seems fairly irrelevant here. In the context of the article the author is acting as a PHP developer. Therefore his actions will viewed as such and any opinions will on his work will (I assume) most likely become referenced to 'PHP developers'. If there was a seemingly large correlation with being jewish and producing crap software, and this article just made that correlation stronger, then yes I would make that comment.
    - xd 14y ago
      You missed my point, big time. It was your sweeping comment lumping all PHP developers as being regarded as crap developers I was getting at. You wouldn't make comments like that about any other groups of people and find it acceptable .. or do you find discrimination acceptable?
daGrevis 14y ago
The title makes me cry!
- EvilTerran 14y ago
  "Improve VB uptime & remove error messages by using 'On Error Resume Next'!"
psaintla 14y ago
I'm going to completely ignore the ridiculousness of the author's claims and stick with talking about @ suppression for a moment... Not this again! I've been working with PHP for over 10 years and seen @ suppression used at companies in the most ridiculous ways imaginable. The most common, and I believe incorrect, way @ suppression is used is when using fopen. Why do people do it so often? Probably because it says you can do so right in the php.net manual http://us2.php.net/manual/en/function.fopen.php http://us2.php.net/manual/en/function.fopen.php and has so for years. I've really lost track of how many bugs I've come across over the years in production code that could be traced back to someone @ suppressing fopen warnings. I don't care if you read some blog where the author claims using @ suppression will give you a magical 1000% increase in performance. JUST DON'T DO IT.
- Gigablah 14y ago
  Another common misuse I see is suppressing notices for non-existent array elements, e.g. @$array['key'].
  - mootothemax 14y ago
    Another common misuse I see is suppressing notices for non-existent array elements, e.g. @$array['key'] Oh wow, that's impressively awful, way more evil than the other common "just turn off notices in php.ini" hack.
  - Sharlin 14y ago
    I don't think that's intrinsically bad. Sometimes you want getting a non-existent key to noisily break, sometimes returning null is a-okay. Many languages have maps/dictionaries with a non-throwing get() operation.
    - phpnode 14y ago
      it is really horrible, just use isset().
      - 14y ago
rickmb 14y ago
And people wonder why serious PHP developers (yes, those exist) avoid Smarty like the plague...
- xd 14y ago
  Understanding why smarty is a bad thing is one of the first steps to PHP enlightenment.
  - TeMPOraL 14y ago
    Could you link to some good material about why smarty should be avoided? I'm eager to read about it. We're using smarty at work now, and while we probably won't drop it in this project (too much working code written in it), we might learn something for the future.
    - phpnode 14y ago
      It'd be better if you told us why you are using Smarty in the first place. I suspect a lot of places use it simply because they've heard a lot of other places use it.
      - z92 14y ago
        My guess is that is the only reason. Otherwise PHP itself is a template language. That is why you start it with <? tag. Just keep short tags on [it's off by default on new installations but ON by default on all hosting sites] and you got a better template engine than smarty.
        jeltz 14y ago
        PHP does not support HTML escaping and is therefor not secure by default. At least twig escapes HTML by default (I am not up to date with PHP so the others might too). You do not want to type <?php echo htmlspecialchars($var, ENT_QUOTES) ?> every time you want to output data. (Yes, I know it could probably be written shorter but my PHP is rusty. My point still remains though, you have to remember to type it every time.)
phpnode 14y ago
There should be a big fat warning in front of this very bad advice, and I particularly dislike the last paragraph about intercepting error handlers to cover up your ugly hack.Some newcomer is going to run with this idea without understanding the many, many problems it can introduce. Until 5.4 the @ operator caused dramatic slowdowns on everything it touched because PHP had to jump through hoops to suppress the errors. It's funny to see someone advocating using it for performance reasons now. If you need a mutex, you should use a mutex. Yet another reason not to use smarty I guess.
- ck2 14y ago
  Define "dramatic slowdowns" ? added: Apparently someone on php.net agrees with you http://php.net/manual/en/language.operators.errorcontrol.php#102543 http://php.net/manual/en/language.operators.errorcontrol.php... they say it takes 1.7 times as long BUT that is only .005 ms in their example
  - sandfox 14y ago
    That example is amongst the worst synthetic benchmarks I have seen in a while. Also it comes from someone who is associated with piwik. A PHP project that makes wordpress look tidy, well engineered and speedy. The more deeply nested the callstack is when you supress, the bigger the performance hit (slight generalisation and slightly from memory).
    - ck2 14y ago
      I spent 10-15 minutes poking around php internals newslist for an explanation of the php 5.4 changelog "improved performance of the (@) silence operator" without luck. It's just listed as a zend engine improvement without specifications. Someone posted here a clear example of why (in earlier php5) it generates far worse opcode: http://derickrethans.nl/five-reasons-why-the-shutop-operator-should-be-avoided.html http://derickrethans.nl/five-reasons-why-the-shutop-operator...
- notum 14y ago
  I'm being offered a "Serbo-Croatian" translation and the site's captcha is broken. Might be unwise to judge authority on the subject just from that, however I decided not to read the article.
ericcholis 14y ago
Forgive me for (possibly) being naive, but why wouldn't you use structured error handing here? PHP's exceptions are great and very extensible.
jeltz 14y ago
He exaggerates the costs of running stat. Stat should not cause any disk IO in the hot case and is therefore quite cheap. Especially since he already need to hit the file system anyway to delete the file. I think most of the work here will happen in memory.
xd 14y ago
If you're on top of Linux I'd suggest as a working alterative without the hackery: http://uk3.php.net/manual/en/book.sem.php http://uk3.php.net/manual/en/book.sem.php
writetehcodez 14y ago
The first paragraph alone hurts my brain. process switching =/= parallel execution multi-threading =/= multi-tasking Only certain single core processors can execute in parallel, so to say that it's possible to execute in parallel on a single core without specifying that not ALL single core CPUs can do it is bad form.
jiggy2011 14y ago
Apart from all of the other considerations outlined here we also have a claim of increased disk performance without any actual evidence that this is the case. Surely file_exists() is simply an operation on the disk index where the relevant block will be sitting in RAM from the first point that the operation is done. Subsequent operations on the same data shouldn't involve hitting the disk again until an actual write has occurred. Provided you have sufficient RAM this sort of thing is what kernel page cache is designed for. Not to mention that thinking about a frequently used performance critical piece of PHP code doing large amounts of @fopen() and @unlink() fills me with dread.
bcoates 14y ago
I don't understand all the hate this article is generating, it's just a basic case of the 'easier to ask forgiveness than permission' vs 'look before you leap' styles. PHP warnings are a debugging tool, not a substitute for exceptions. Suppressing them is not the equivalent of a catching and ignoring an exception. Putting locks everywhere in a futile attempt to prevent the filesystem from changing under you is futile. It's fundamentally an external i/o device out of your program's control. It is never safe or reasonable to infer from the result of one filesystem operation that a subsequent one will not fail.

32 points by willmacdonald 14y ago

Maybe I didn't understand this article properly, but is it suggesting suppressing ALL filesystem related errors rather than handling them properly? OK I get that PHP doesn't support atomic operations, but there are so many different errors that could happen when working with files. Say a permissions error, if the permissions of the file were changed between it being created and deleted that's an issue that needs resolving before your tmp folder fills up, it's easy enough to fix and should be reported as an error (be it to the front-end or to a backend logging service). Surely there is a better way to handle these race conditions without just ignoring all filesystem related errors.

RossM 14y ago
No, I think the author knows the issues behind this muting - you just need to be smart about how you apply it. In the case of filemtime, it appears the only failure condition is a file not existing so we can safely ignore the E_WARNING it emits; it wouldn't be sensible to apply this everywhere (I hadn't thought about the permissions case myself). The best/real solution is probably implementing flock so that it can issue real filesystem locks, rather than relying on a file resource.

sandfox 14y ago

And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software. It's a broken solution to a problem that shouldn't really be there. Ignoring why the race condition is happening, what is wrong with something like Redis or heaven forbid semaphores/shared memory for locking. There are billions of ways of doing atomic updates right and this is not one of them --EDIT I hadn't even got as far as the disk i/o section, after reading no words can do justice to the complete lack of understanding shown here with regards to filesystems, caching, SSD's and linux.

Gigablah 14y ago
Not to mention you wouldn't even have a need for this so-called "optimization" in production, since you'd have your templates precompiled and cached (and turned the compile check off).
mootothemax 14y ago
And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software. It's a broken solution to a problem that shouldn't really be there. Very much agree with you. When I was first starting out programming commercially, one of the best bits of advice I received was something along the lines of "Take a step back and think about why you're using these crazy constructs - chances are there's a much simpler way of doing things." The lesson being: if you're having to write code like this, chances are it's because you're creating a mess. Take a step back and work out a simpler solution.
xd 14y ago
"And this is another reason why everyone regards PHP developers as being a tinsy bit crap at developing software." Replace "PHP" with "Jewish" and would you find that comment of yours acceptable? Edit: I can only assume that the down voters find discrimination acceptable behavior in this community.
- sandfox 14y ago
  Trollolol much? The religion of the author (is he even jewish? - I have no idea nor really care) seems fairly irrelevant here. In the context of the article the author is acting as a PHP developer. Therefore his actions will viewed as such and any opinions will on his work will (I assume) most likely become referenced to 'PHP developers'. If there was a seemingly large correlation with being jewish and producing crap software, and this article just made that correlation stronger, then yes I would make that comment.
  - xd 14y ago
    You missed my point, big time. It was your sweeping comment lumping all PHP developers as being regarded as crap developers I was getting at. You wouldn't make comments like that about any other groups of people and find it acceptable .. or do you find discrimination acceptable?

daGrevis 14y ago

The title makes me cry!

EvilTerran 14y ago
"Improve VB uptime & remove error messages by using 'On Error Resume Next'!"

psaintla 14y ago

I'm going to completely ignore the ridiculousness of the author's claims and stick with talking about @ suppression for a moment... Not this again! I've been working with PHP for over 10 years and seen @ suppression used at companies in the most ridiculous ways imaginable. The most common, and I believe incorrect, way @ suppression is used is when using fopen. Why do people do it so often? Probably because it says you can do so right in the php.net manual http://us2.php.net/manual/en/function.fopen.php http://us2.php.net/manual/en/function.fopen.php and has so for years. I've really lost track of how many bugs I've come across over the years in production code that could be traced back to someone @ suppressing fopen warnings. I don't care if you read some blog where the author claims using @ suppression will give you a magical 1000% increase in performance. JUST DON'T DO IT.

Gigablah 14y ago
Another common misuse I see is suppressing notices for non-existent array elements, e.g. @$array['key'].
- mootothemax 14y ago
  Another common misuse I see is suppressing notices for non-existent array elements, e.g. @$array['key'] Oh wow, that's impressively awful, way more evil than the other common "just turn off notices in php.ini" hack.
- Sharlin 14y ago
  I don't think that's intrinsically bad. Sometimes you want getting a non-existent key to noisily break, sometimes returning null is a-okay. Many languages have maps/dictionaries with a non-throwing get() operation.
  - phpnode 14y ago
    it is really horrible, just use isset().
    - 14y ago

rickmb 14y ago

And people wonder why serious PHP developers (yes, those exist) avoid Smarty like the plague...

xd 14y ago
Understanding why smarty is a bad thing is one of the first steps to PHP enlightenment.
- TeMPOraL 14y ago
  Could you link to some good material about why smarty should be avoided? I'm eager to read about it. We're using smarty at work now, and while we probably won't drop it in this project (too much working code written in it), we might learn something for the future.
  - phpnode 14y ago
    It'd be better if you told us why you are using Smarty in the first place. I suspect a lot of places use it simply because they've heard a lot of other places use it.
    - z92 14y ago
      My guess is that is the only reason. Otherwise PHP itself is a template language. That is why you start it with <? tag. Just keep short tags on [it's off by default on new installations but ON by default on all hosting sites] and you got a better template engine than smarty.
      - jeltz 14y ago
        PHP does not support HTML escaping and is therefor not secure by default. At least twig escapes HTML by default (I am not up to date with PHP so the others might too). You do not want to type <?php echo htmlspecialchars($var, ENT_QUOTES) ?> every time you want to output data. (Yes, I know it could probably be written shorter but my PHP is rusty. My point still remains though, you have to remember to type it every time.)

phpnode 14y ago

There should be a big fat warning in front of this very bad advice, and I particularly dislike the last paragraph about intercepting error handlers to cover up your ugly hack.Some newcomer is going to run with this idea without understanding the many, many problems it can introduce. Until 5.4 the @ operator caused dramatic slowdowns on everything it touched because PHP had to jump through hoops to suppress the errors. It's funny to see someone advocating using it for performance reasons now. If you need a mutex, you should use a mutex. Yet another reason not to use smarty I guess.

ck2 14y ago
Define "dramatic slowdowns" ? added: Apparently someone on php.net agrees with you http://php.net/manual/en/language.operators.errorcontrol.php#102543 http://php.net/manual/en/language.operators.errorcontrol.php... they say it takes 1.7 times as long BUT that is only .005 ms in their example
- sandfox 14y ago
  That example is amongst the worst synthetic benchmarks I have seen in a while. Also it comes from someone who is associated with piwik. A PHP project that makes wordpress look tidy, well engineered and speedy. The more deeply nested the callstack is when you supress, the bigger the performance hit (slight generalisation and slightly from memory).
  - ck2 14y ago
    I spent 10-15 minutes poking around php internals newslist for an explanation of the php 5.4 changelog "improved performance of the (@) silence operator" without luck. It's just listed as a zend engine improvement without specifications. Someone posted here a clear example of why (in earlier php5) it generates far worse opcode: http://derickrethans.nl/five-reasons-why-the-shutop-operator-should-be-avoided.html http://derickrethans.nl/five-reasons-why-the-shutop-operator...
notum 14y ago
I'm being offered a "Serbo-Croatian" translation and the site's captcha is broken. Might be unwise to judge authority on the subject just from that, however I decided not to read the article.

ericcholis 14y ago

Forgive me for (possibly) being naive, but why wouldn't you use structured error handing here? PHP's exceptions are great and very extensible.

jeltz 14y ago

He exaggerates the costs of running stat. Stat should not cause any disk IO in the hot case and is therefore quite cheap. Especially since he already need to hit the file system anyway to delete the file. I think most of the work here will happen in memory.

xd 14y ago

If you're on top of Linux I'd suggest as a working alterative without the hackery: http://uk3.php.net/manual/en/book.sem.php http://uk3.php.net/manual/en/book.sem.php

writetehcodez 14y ago

The first paragraph alone hurts my brain. process switching =/= parallel execution multi-threading =/= multi-tasking Only certain single core processors can execute in parallel, so to say that it's possible to execute in parallel on a single core without specifying that not ALL single core CPUs can do it is bad form.

jiggy2011 14y ago

Apart from all of the other considerations outlined here we also have a claim of increased disk performance without any actual evidence that this is the case. Surely file_exists() is simply an operation on the disk index where the relevant block will be sitting in RAM from the first point that the operation is done. Subsequent operations on the same data shouldn't involve hitting the disk again until an actual write has occurred. Provided you have sufficient RAM this sort of thing is what kernel page cache is designed for. Not to mention that thinking about a frequently used performance critical piece of PHP code doing large amounts of @fopen() and @unlink() fills me with dread.

bcoates 14y ago

I don't understand all the hate this article is generating, it's just a basic case of the 'easier to ask forgiveness than permission' vs 'look before you leap' styles. PHP warnings are a debugging tool, not a substitute for exceptions. Suppressing them is not the equivalent of a catching and ignoring an exception. Putting locks everywhere in a futile attempt to prevent the filesystem from changing under you is futile. It's fundamentally an external i/o device out of your program's control. It is never safe or reasonable to infer from the result of one filesystem operation that a subsequent one will not fail.