19

In a function that reads data (data meaning exclusively strings) from disk, which should I prefer? Which is better?

A) DiskStream.Read(Pointer(s)^, Count)
or
B) DiskStream.Read(s[1], Count)

Note:
I know both are having the same result.
I know that I have to SetLength of S before calling Read.

UPDATE

S is AnsiString.

Here is the full function:

{ Reads a bunch of chars from the file. Why 'ReadChars' and not 'ReadString'? This function reads C++ strings (the length of the string was not written to disk also). So, i have to give the number of chars to read as parameter. }

function TMyStream.ReadChars(out s: AnsiString; CONST Count: Longint): Boolean; 
begin
 SetLength(s, Count);
 Result:= Read(s[1], Count)= Count;
end;

Speed test

In my speed test the first approach was a tiny bit faster than the second one. I used a 400MB file from which I read strings about 200000 times. The process was set to High priority.

The best read time ever was:
1.35 for variant B and 1.37 for variant A.
Average:
On average, B was scoring also 20ms better than A.

The test was repeated 15 times for each variant.

The difference is really small. It could fall into the measuring error range. Probably it will be significant if I read strings more often and from a bigger file. But for the moment let's say that both lines of code are performing the same.

ANSWER
Variant A - might be a tiny tiny bit faster Variant B - is (obviously) much more easier to read and it is more Delphi-ish. My preferred.

Note:
I have seen Embarcadero using variant A in TStreamReadBuffer example, but with a TBytes instead of String.

Gabriel
  • 20,797
  • 27
  • 159
  • 293
  • 1
    Which Delphi version is this? You could run into trouble with this approach in D2009+ because of the Unicode changes. This article of Nick Hodges clearifies how it should be done in the newer Delphi versions: http://edn.embarcadero.com/article/38693 – Jens Mühlenhoff Jun 20 '11 at 12:56
  • Hi Jens. I forgot about that. I updated my question to show that s is AnsiString. – Gabriel Jun 20 '11 at 13:08
  • 1
    Just saw your updated speed test. I'd say 20 ms is a price worth paying for better code readability and avoiding any problems that might arise from not calling `UniqueString`, wouldn't you? – Mason Wheeler Jun 20 '11 at 13:41
  • I hate it when these questions descend into arcane discussions of dubious optimisations. Why do we seem to be so much more enticed by performance than correctness? Clearly both are important but I really strongly feel that correctness has to come first. – David Heffernan Jun 20 '11 at 13:50
  • 1
    @Altar: You say the difference would be more significant if you read more strings from a bigger file. Think about that for a second. You're reading a 400 MB file. Multiply everything by 10 and you have a difference of 0.2 seconds for a 4 GB file. You're running into the limitations of the 32 bit address space now, and you're still only saving a negligible fraction of a second. It's really not worth it. – Mason Wheeler Jun 20 '11 at 13:59
  • @Mason - you are considering only bigger files and not also reading strings more often from the file. What if instead of 200000 strings, the files contains 10 mil strings or more? But anyway, I DO AGREE that for the way I read the file now (they don't have lots of strings inside), the difference is NOT significant. – Gabriel Jun 20 '11 at 14:04

7 Answers7

18

Definitely the array notation. Part of Delphi style is to make your code easy to read, and it's easier to tell what's going on when you spell out exactly what you're doing. Casting a string to a pointer and then dereferencing it looks confusing; why are you doing that? It doesn't make sense unless the reader knows a lot about string internals.

Mason Wheeler
  • 82,511
  • 50
  • 270
  • 477
  • Hi Mason. I am actually using the second one. I was asking because if you do a Google search you will see that Delphi programmers are using the first one quite often. So, I was looking for a reason. – Gabriel Jun 20 '11 at 12:54
  • 4
    @Altar: The reason is mostly that they think it's faster. But as you saw when you tested it, the performance increase is negligible. When talking about speed, a good rule of thumb is "it's not any slower unless the user will notice the difference." And no one's going to notice 20 ms either way, especially out of a total time of less than 1.5 seconds. – Mason Wheeler Jun 20 '11 at 13:44
  • 1
    You are right. Initially I also thought they have a reason. Now, after performing the test I can see the real numbers. 'Accepted'. – Gabriel Jun 20 '11 at 14:06
  • PS: the variant B was always my preferred and I always used it in my code. I just wanted to see why people are using the first variant. – Gabriel Jun 20 '11 at 14:17
14

Be aware that when running

1. DiskStream.Read(Pointer(s)^, Count)
2. DiskStream.Read(s[1], Count)

The 1. version will be faster.

But you must be sure that the s variable is explicitly local, or you have called yourself UniqueString(s) before the loop.

Since pointer(s)^ won't call UniqueString?() low-level hidden RTL call, it will be faster than s[1], but you may override some existing data if the s string variable is shared between the current context and other context (e.g. if the last content of s was retrieved from a function from a property value, or s is sent as parameter to another method).

In fact the fastest correct way of coding this reading an AnsiString from content is:

  s := '';
  SetLength(s,Count);
  DiskStream.Read(pointer(s)^,Count);

or 

  SetString(s,nil,Count);
  DiskStream.Read(pointer(s)^,Count);

The 2nd version being equal to the 1st, but with one line less.

Setting s to '' will call FreeMem()+AllocMem() instead of ReallocMem() in SetLength(), so will avoid a call to move(), and will be therefore a bit faster.

In fact, the UniqueString?() RTL call generated by s[1] will be very fast, since you have already called SetLength() before calling it: therefore, s is already unique, and UniqueString?() RTL call will return almost immediately. After profiling, there is not much speed difference between the two versions: almost all time is spend in string allocation and content moving from disk. Perhaps s[1] is found to be more "pascalish".

Arnaud Bouchez
  • 42,305
  • 3
  • 71
  • 159
  • 2
    +1 I don't believe performance is really the issue here, but I give you much credit for dealing accurately with the correctness issue regarding `UniqueString`. To me the complexity of ths answer is a perfect justification for selecting `s[1]`. ;-) – David Heffernan Jun 20 '11 at 14:07
  • I have changed the parameter from 'var S: string' to 'our s: string'. I think this will solve the s:= '' issue. – Gabriel Jun 20 '11 at 14:10
  • @Altar What do you mean with 'our s:string'? `s := '';` is to avoid a realloc call if you call SetLength() multiple times on the same variable, e.g. in a loop. – Arnaud Bouchez Jun 20 '11 at 17:50
  • 1
    @David Thanks for your input. We both agree that `pointer(s)` should be used only if you know what you're doing. I wanted to emphasize that it could be **dangerous**! So if a Delphi coder as the smallest doubt about using `pointer(s)`, he/she should **avoid** it and rely on `s[1]`! ;) – Arnaud Bouchez Jun 20 '11 at 17:52
  • @Bouchez - sorry. I meant OUT instead of OUR. Like in: procedure xyz(out i: integer) – Gabriel Jun 24 '11 at 12:15
7

If you care about optimization you should prefer the first variant. Just look at the code generated by compiler:

Unit7.pas.98: Stream.Read(Pointer(S)^, 10);
00470EA9 8B55FC           mov edx,[ebp-$04]
00470EAC B90A000000       mov ecx,$0000000a
00470EB1 8BC6             mov eax,esi
00470EB3 8B18             mov ebx,[eax]
00470EB5 FF530C           call dword ptr [ebx+$0c]

Unit7.pas.99: Stream.Read(s[1], 10);
00470EB8 8B5DFC           mov ebx,[ebp-$04]
00470EBB 85DB             test ebx,ebx
00470EBD 7418             jz $00470ed7
00470EBF 8BC3             mov eax,ebx
00470EC1 83E80A           sub eax,$0a
00470EC4 66833802         cmp word ptr [eax],$02
00470EC8 740D             jz $00470ed7
00470ECA 8D45FC           lea eax,[ebp-$04]
00470ECD 8B55FC           mov edx,[ebp-$04]
00470ED0 E8CB3FF9FF       call @InternalUStrFromLStr
00470ED5 8BD8             mov ebx,eax
00470ED7 8D45FC           lea eax,[ebp-$04]
00470EDA E89950F9FF       call @UniqueStringU
00470EDF 8BD0             mov edx,eax
00470EE1 B90A000000       mov ecx,$0000000a
00470EE6 8BC6             mov eax,esi
00470EE8 8B18             mov ebx,[eax]
00470EEA FF530C           call dword ptr [ebx+$0c]

UPDATE

The above code is generated by Delphi 2009 compiler. You can improve the code by using {$STRINGCHECKS OFF} directive, but you still have UniqueStringU function call overhead:

Unit7.pas.100: Stream.Read(s[1], 10);
00470EB8 8D45FC           lea eax,[ebp-$04]
00470EBB E8B850F9FF       call @UniqueStringU
00470EC0 8BD0             mov edx,eax
00470EC2 B90A000000       mov ecx,$0000000a
00470EC7 8BC3             mov eax,ebx
00470EC9 8B18             mov ebx,[eax]
00470ECB FF530C           call dword ptr [ebx+$0c]
kludg
  • 27,213
  • 5
  • 67
  • 118
  • 1
    Which Delphi version is this, do you have the StringChecks option enabled (if applicable)? – Jens Mühlenhoff Jun 20 '11 at 13:01
  • WOW! There is a lot of code for the second one. Now I understand why people (including Embarcadero) are using the first variant. I will immediately switch to the first variant! – Gabriel Jun 20 '11 at 13:05
  • Update: I see that your code is using Unicode strings. I forgot to mention that I use AnsiStrings. – Gabriel Jun 20 '11 at 13:08
  • 1
    Interesting. Nevertheless this answer ignores the "What is more Delphi Style" part of the question. And the few more operations doesn't matter in 99,9 percent. – jpfollenius Jun 20 '11 at 13:18
  • Hi Smasher. You are right. But I was not aware that there is a such big difference between those two lines of code. In this light I have changed my question. I will also make a speed test to see if there is any difference in speed. – Gabriel Jun 20 '11 at 13:25
  • @Altar: that's alright. But note that the "big difference" is reduced to one function call with `StringChecks` disabled. – jpfollenius Jun 20 '11 at 13:31
  • 1
    @Altar: Do you know what order of magnitude the clock frequency of a modern CPU is? – Andreas Rejbrand Jun 20 '11 at 13:32
  • 1
    @Altar: That UniqueString function call isn't "overhead"; it's a very important bit of correctness. Since strings are reference counted, if your string happens to be used in more than one place, not calling UniqueString will stomp the other references as well. (And if it's not used in more than one place, UniqueString will return so quickly that you'll never notice the call overhead anyway.) – Mason Wheeler Jun 20 '11 at 13:32
  • 2
    @Altar, The performance difference would be negligible. Always go for readability over performance, unless the (quantified) performance gain justifies the decreased readability. The real "cost" of software is bug-fixing and maintenance - rarely is performance the real issue, and in almost all of those cases we are not talking of just a few percent. – Misha Jun 20 '11 at 13:32
  • 2
    @Mason - if `DiskStream.Read(Pointer(s)^, Count)` is correct, then `UniqueString` is overhead. Sure if you write `Pointer(s)^` you should understand what are doing, or else you can have side effects. – kludg Jun 20 '11 at 13:42
  • 1
    @Andreas Rejbrand, actually clock frequency is on decline. – Premature Optimization Jun 20 '11 at 13:42
  • 1
    @Altar, I would think that the 0.02 seconds is about the margin of error (+/- 2% is about as accurate as you could hope for), so to all intents and purposes, both variants take the same amount of time – Misha Jun 20 '11 at 13:50
  • I will redo the test with a larger file. – Gabriel Jun 20 '11 at 13:57
  • 1
    @Mason, the UniqueString call in this case *is* overhead; it comes immediately after a call to SetLength, which already always yields a unique string. It would be nice if the code generator were more aware of the side effects of its own built-in functions. – Rob Kennedy Jun 20 '11 at 14:00
  • 1
    Did you have range checking enabled, Serg? *That's* the reason I prefer the first variant; I don't want a range-checking error when `Count` is zero and `s[1]` doesn't exist. It's been a while since I've actually checked whether the compiler does range checking in that situation, though. – Rob Kennedy Jun 20 '11 at 14:06
  • @Rob It does indeed give a range check error if you try s[1] on an empty string. – David Heffernan Jun 20 '11 at 14:16
  • @Rob - No, range checking is disabled. It is an effect of `$STRINGCHECK` directive which is `ON` by default in Delphi 2009. – kludg Jun 20 '11 at 14:37
6

The second option is definitely more "Delphi style" (if you look at the Delphi versions of the Windows API headers, you will see that most pointer parameters have been converted to var parameters).

In addition to that, the second option does not need a cast and is much more readable IMHO.

jpfollenius
  • 16,456
  • 10
  • 90
  • 156
  • In the Help page for TStreamReadBuffer, I see that Embarcadero is using this: Stream.ReadBuffer(Pointer(Buffer)^, Size) (It is true though that Buffer is not a string but TBytes). – Gabriel Jun 20 '11 at 13:21
  • 1
    IMO, Borland API header translations should be used as model, there are lot of *mispascalized* prototypes, which requires client to invoke as `fn(PDWORD(nil)^)`. – Premature Optimization Jun 20 '11 at 13:22
  • @Downvoter: I guess you mean "not used as a model"? Might be badly translated in some cases but certainly not in the majority. – jpfollenius Jun 20 '11 at 13:26
  • 1
    @Smasher, LOL, yes, **header translations should NOT be used**, got really confused while constructing that scary reference-cast-deref pattern :) Unfortunately, there are lot of such. And the numerous cases of opposite (eg: ultimately *nonpascalized* WinSock) – Premature Optimization Jun 20 '11 at 13:31
  • @Downvoter Recent versions of Delphi have dealt with many of these mispascalized prototypes. Are you still on Delphi 7 by any chance? – David Heffernan Jun 20 '11 at 14:19
  • @David Heffernan, probably, definitely not all of them (as in D210) – Premature Optimization Jun 20 '11 at 14:24
5

I'd always use the second one which maintains type safety. I don't really buy the performance argument since you are about to hit the disk at worst, or file cache, or main memory, all of which are going to make a handful of CPU operations look somewhat trivial. Correctness should be given higher priority than performance.

However, I would add that this is not something that should be bothering you too much since you should write this particular piece of code once and once only. Put it in a helper class and wrap it up well. Feel free to care about optimisation, re-write it as assembler, whatever takes your fancy. But don't repeat yourself.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • "I don't really buy the performance argument since you are about to hit the disk" - - - You are pretty much right about that except that Windows will cache the file for you, so on the second pass you will not read from disk anymore but from Windows cache (RAM). The speed optimization might be real. – Gabriel Jun 20 '11 at 13:18
  • @Altar Even with the cache, the overhead of calling ReadFile will far outweigh the savings that A.Bouchez talks about. Now, if you are [doing the buffering yourself](http://stackoverflow.com/questions/5639531/buffered-files-for-faster-disk-access/5639712#5639712) then the overhead is much reduced. SetLength is liable to be much more costly too. – David Heffernan Jun 20 '11 at 13:24
  • 2
    @Altar: If you want to really speed up disc access, don't ever stream a file from disc through a routine like this. Instead, write a routine that opens a file in a TFileStream, then reads the entire contents all at once into a TMemoryStream, and use the TMemoryStream to do your loading. The performance gain is pretty spectacular. (Of course, this could cause problems with really enormous files, and you'll need to try more complicated tricks. But for most stuff it's a wonderful, simple speed boost.) – Mason Wheeler Jun 20 '11 at 13:36
  • @David: I think you meant @Serg as the author of the referenced answer, not A.Bouchez. – jpfollenius Jun 20 '11 at 13:36
  • @Mason: +1, but there are two situations that should be mentioned where this is not a good approach: (a) if you do not always need to read the whole file and (b) if files are very big – jpfollenius Jun 20 '11 at 13:38
  • @Smasher You are right, but sure enough @A.Bouchez has just appeared to make my comment factually correct! – David Heffernan Jun 20 '11 at 13:39
  • @Mason @Smasher TMemoryStream is a bad idea because it requires contiguous address space. In fact if you follow the link in my comment above you will see that I am well aware of the issues involved and have code that deals with address space limitations too. In fact that code was put on SO in response to a question from @Altar! – David Heffernan Jun 20 '11 at 13:41
  • @David - I use indeed your code to read data from large files. The code works like a charm. But I still have to pass strings to it (method A or B). Thanks again for it. And +1. – Gabriel Jun 20 '11 at 13:54
4

If there is ever any chance that your function will be called with a Count of 0, then A) will work with Pointer(s)^ simply evaluating to nil while B) will crash with a range check exception.

If you want to use B) and still handle counts of 0 gracefully, you should use:

function TMyStream.ReadChars(out s: AnsiString; const Count: Integer): Boolean; 
begin
 SetLength(s, Count);
 Result := (Count = 0)  or (Read(s[1], Count) = Count);
end;
Thorsten Engler
  • 2,333
  • 12
  • 13
  • Thanks for mentioning that. The binary file that I read should not have empty strings. But as you say, I better check it. – Gabriel Jun 24 '11 at 12:17
1

The second one (DiskStream.Read(s[1], Count)). Whenever you encounter an untyped var parameter it reads like "take the address of what is passed as a parameter". So in this case you are passing the address of the first character of the string s, which is what you intended to do.

Misha
  • 1,816
  • 1
  • 13
  • 16