Is it faster to check a file's existence prior to loading, or to catch the exception when it doesn't exist?

Question

I have been recommended to use the second, try-except variant, but I would also like to know what others think: which procedure of the two below (if any) is more time-efficient?

procedure LoadImage(img: TImage; filename: string);
begin
  if fileexists(filename) then
    img.Picture.Loadfromfile(filename)
  else
    img.Picture.Loadfromfile('default.jpg')
end;

or

procedure LoadImage(img: TImage; filename: string);
begin
  try
    img.Picture.Loadfromfile(filename)
  except
    img.Picture.Loadfromfile('default.jpg')
  end
end;

They're not equivalent. The first one has at least two issues: 1) A race condition 2) If the file exists, but cannot be loaded it'll throw an exception. — CodesInChaos, Aug 10 '12 at 12:27
How often does the first image exist? The first code optimizes the error case, the second the success case. — CodesInChaos, Aug 10 '12 at 12:29
if img.Picture.Loadfromfile(filename) gives an exception there might be other things that threw the exception then the absence of the file and the object will be in an undefined state. That's why I wouldn't recommend the second method. — Pieter B, Aug 10 '12 at 12:30
it is questionable whether the difference is actually measurable. The time required by file i/o and constructing the object will be larger by orders of magnitude than the time consumed by the try/except block. — niko, Aug 10 '12 at 12:32
for god's sake, you're loading an image.. don't micro-optimize code which is orders of magnitude faster. — Karoly Horvath, Aug 10 '12 at 12:33
@DavidHeffernan "Such code" being code that first checks whether the file exists and then opens it -- in other words, code that has that race condition? Yes, code that has that race condition has that race condition. Code that attempts to open and catches and handles `EFileNotFoundException` doesn't. — , Aug 10 '12 at 12:43
it's not either/or, you should both check whether the file exists and catch errors. I really don't like having program flow defined through try excepts. — Pieter B, Aug 10 '12 at 13:03
@PieterB Then use `AssignFile`, set `{$I-}`, call `Reset`, and check `IOResult`. :) — , Aug 10 '12 at 13:10
@hvd the else condition loads a different file so that's really the race I was thinking of — David Heffernan, Aug 10 '12 at 14:42
Just a recommendation, if you plan to load `Default.jpg` many times, then it might be a good idea to pre-load this image once and leave it loaded, using the already loaded version every time it's needed, rather than loading it from the file over and over. — Jerry Dodge, Aug 10 '12 at 15:34
@DavidHeffernan There's only one race condition that I can see, and that's the possibility that the existence of the file changes between the `FileExists` and the `LoadFromFile` calls. There are other issues, but none of those are race conditions as far as I can tell. Could you elaborate? — , Aug 10 '12 at 22:32
@hvd There's always race conditions with the file system. Other processes can modify the files. Both versions of the code in the Q suffer from that. — David Heffernan, Aug 12 '12 at 19:39
@DavidHeffernan I'm not sure that can be called a race condition, but it is indeed a good point and a reason why code might in the general case fail regardless of what you call it. It can be avoided by opening with deny-write access, but I cannot check right now whether `TPicture.LoadFromFile` does so. — , Aug 12 '12 at 20:56
@hvd It's true that the race condition you point out is very clear and the code emphasises it. But it still relies on interference from another process. And once you accept that possibility then all file open operations are subject to race. That's really my point. So I would always just treat that condition as an unavoidable hazard and disregard it. — David Heffernan, Aug 13 '12 at 07:55
@DavidHeffernan Yet that's my point: file operations don't have to be subject to race conditions. It is avoidable. Even another process attempting to modify the file while you're reading it. — , Aug 13 '12 at 08:01
@hvd Once LoadFromFile returns, the file is unlocked and the other process can do what it wants. So there's a race no matter what. By the time you've handled the exception from LoadFromFile and displayed the error message to the user, the file could be there. Also, since these files are presumably only meant to be manipulated by this program, why do you need to worry about stuff like this? I'd always prefer to open the file and then deal with the exception if it occurred. But that way still has a race but my point is that the race does not matter. It is benign. — David Heffernan, Aug 13 '12 at 11:10
@DavidHeffernan First part: that's not a problem. If LoadFromFile raises an exception, and the file does exist by the time the error is shown, showing the error isn't wrong, because at the time the file load was attempted, the file really didn't exist. Second part: I only assume `'default.jpg'` is managed by the program, and for that I don't worry about it. For `filename`, I see no basis for that assumption. Third part: then we're in agreement about the way to write code, just not about the reason why. :) — , Aug 13 '12 at 11:26

score 8 · Answer 1 · answered Aug 10 '12 at 13:17

8

Forget efficiency. Code readability is way, way more important. Premature optimization is the root of all sorts of evil.

The first one is clear in its intentions. Everyone can easily figure out what is up.

The second one makes me stop and go "What the....?"

You never want your code to cause the second reaction.

answered Aug 10 '12 at 13:17

Nick Hodges

16,902
11
68
130

2

Clear incorrect code should never be preferred over less clear correct code. And no, I'm not saying the second one is correct. – Aug 10 '12 at 13:20
@hvd: "Clear incorrect code should never be preferred over less clear correct code." -- that goes without saying. But of course, correct *and* clear code is the best. – Nick Hodges Aug 10 '12 at 13:39
Fully agreed. I commented because neither the first nor the second is both, and I read your answer as saying the first *is* correct. – Aug 10 '12 at 13:40
It depends on the desired semantics if you consider the first correct. Fallback-on-error and fallback-on-missing-file are different, and I don't know which one the OP wants. – CodesInChaos Aug 10 '12 at 13:44
@CodesInChaos I was referring to the race condition you mentioned, which makes it incorrect even for follback-on-missing-file. – Aug 10 '12 at 13:53
5

You haven't answered the question. It's fine to complain that the question was even asked in the first place, but if the complaint isn't going to be accompanied by an answer, then it should be in a comment, no matter how well argued it is. – Rob Kennedy Aug 10 '12 at 14:42
Rob -- I thought I did answer the question: The first one is more efficient because it is more readable and maintainable. – Nick Hodges Aug 27 '12 at 14:41

score 4 · Answer 2 · answered Aug 10 '12 at 13:07

4

If time efficiency is your only criteria, first one will be faster because exception handling is CPU consuming.

FileExists() uses one WinApi call so its fast but it checks only if file exists. If file exists but its in wrong format or its blocked by other thread you will get unhandled exception.

answered Aug 10 '12 at 13:07

Jarek Bielicki

856
6
16

1

Are you certain the overhead of `FileExists()` is smaller than that of exception handling? Even if the path may be an UNC path to a slow server? (The difference will *probably* be small enough that it really doesn't matter which is faster.) – Aug 10 '12 at 13:13
1

Exception handling is only expensive if an exception actually is thrown. The try .. except code itself does not slow down the execution. – mjn Aug 10 '12 at 13:18
@mjn At least in old versions of delphi entering a `try...except` clause does slow down execution. Probably negligible compared to the cost of IO, but you don't want a `try except` in a tight loop, even if it never actually throws. – CodesInChaos Aug 10 '12 at 13:22
1

In Delphi 4 and Delphi 2005, @Codes, entering a try-except block involved two stack pushes and a mov, and I don't think it's changed since. How old are you talking about? – Rob Kennedy Aug 10 '12 at 14:32
Probably Delphi 4 or 6. From what I remember try-except (or perhaps it was try-finally) involved calls to expensive windows API functions related to structured exception handling. – CodesInChaos Aug 10 '12 at 14:43
I can think of only two API functions related to structured exception handling, @Codes. They are `RaiseException` and `RtlUnwind`. Obviously, neither is used for entering any `try` block, and neither is used without an exception being raised, either. In non-exceptional circumstances, `try` blocks are about as close to free as you can expect to get. – Rob Kennedy Aug 10 '12 at 20:11
On 64 bit there no longer is a performance penalty using exceptions. – Marjan Venema Aug 11 '12 at 13:23

CodesInChaos · Answer 3 · 2012-08-10T13:42:42.257

0

One problem with your question is that you didn't specify the desired semantics. There are two possible interpretations, and which solution is better, depends on that choice. I assume being unable to load the fallback, is a fatal error.

If the first file does exist, load it, else load the second. If the first file exists, but could not be loaded, show an error.
If the first file cannot be loaded, fallback to the second.

If you want the semantics of 1) your first code is fine.

If you want the semantics of 2), neither is good. The first code doesn't have these semantics because:

It has a race condition. If the image gets deleted between checking the existence of the file and loading it, it'll fail.
If the file exists, but cannot be loaded, an exception will be thrown. This can happen if the file is no valid image, or cannot be opened.

The second one uses exceptions for a common case, which is bad.

So to achieve the second semantics I'd use:

procedure LoadImage(img: TImage; filename: string);
var success:boolean;
begin
  success := false;
  if FileExists(filename)
    then try
      img.Picture.LoadFromFile(filename);
      success := true;
    except
    end
  if not success
    then img.Picture.LoadFromFile('default.jpg');
end;

Checking for existence before opening makes the error case faster, but the success case slower. So which one is faster depends on your usage.

Personally I'd use the third variant over the second, even if the image is only missing occasionally, since I believe a normally operating application should not throw exceptions. I'd only care about the performance issue if benchmarking revealed it to be noticeable.

You should also consider a more targeted exception clause. Blanket catching all exception is bad style. Unfortunately I couldn't find a clear specification which exceptions are thrown by TPicture.LoadFromFile, so I'll leave in the blanket clause for now.

edited Aug 10 '12 at 13:42

answered Aug 10 '12 at 12:34

CodesInChaos

106,488
23
218
262

1

Now if file does not exists procedure will do nothing. So its not the best tip – Jarek Bielicki Aug 10 '12 at 12:36
2

Now you first load from filename, and then ditch that to load default.jpg. Missing `else`. – Aug 10 '12 at 12:44
2

I wouldn't use an empty except clause, that's asking for trouble. – Pieter B Aug 10 '12 at 13:10
9

NEVER EVER EVER EVER EVER EVER EVER EVER EVER EVER EVER EVER EVER leave and except clause empty. EVER! – Nick Hodges Aug 10 '12 at 13:14
2

@NickHodges The code does exactly the same thing as it would if it still contained `try LoadFromFile(filename); except LoadFromFile('default.jpg'); end;`. I agree that `except end` is bad, but `except LoadFromFile('default.jpg'); end` is equally bad. – Aug 10 '12 at 13:18
2

I recommended more specific exception filtering, so it only catches IO related exceptions. But I couldn't find a statement in the documentation stating which exceptions this method might throw. Do you have a good source for that? But leaving it empty isn't any worse than what's done in the original question. – CodesInChaos Aug 10 '12 at 13:19
if the file exists but is in use, the first one from asker will throw an exception on LoadFromFile(filename), the second one will LoadFromFile('default.jpg') – Pieter B Aug 10 '12 at 13:27
@PieterB That's one special case of the second issue with the first piece of code I mentioned. – CodesInChaos Aug 10 '12 at 13:30
@NickHodges - you could even add bold font to caps lock but it still does not explain why empty except block is bad. – kludg Aug 10 '12 at 13:57
empty except block are bad because they suppress all errors and leave objects in an undefined state. Objects in an undefined state can be the cause of many strange bugs. – Pieter B Aug 10 '12 at 13:59
@PieterB - I don't understand what objects and undefined states are you talking about. – kludg Aug 10 '12 at 14:08
@serg when loadfromfile throws an exception that exception can be for a lot of reasons, not all fileloading related. In what state will the picture object be? Has the image already been loaded or not? You honestly can't tell because the exception got eaten. – Pieter B Aug 10 '12 at 14:21
1

@PieterB right if `loadfromfile` throws AV everything is possible; but that is not empty except block that potentially makes your program unstable in this case - you can't handle every possible exception in a real project. The bad of empty except is that it hides the problem. – kludg Aug 10 '12 at 14:38
Catch, try to handle and if else re-raise the exception. – John Easley Aug 10 '12 at 16:33
@Pieter your argument is essentially that if an exception is raised by an object then you cannot call any more methods on that object. That's clearly nonsense. – David Heffernan Aug 10 '12 at 16:53
@David no, I mean catching the known exceptions but let the rest go through. – Pieter B Aug 10 '12 at 21:18

Is it faster to check a file's existence prior to loading, or to catch the exception when it doesn't exist?

3 Answers3