Why does my code run slower if I call it from a separate thread?

Question

I've just added threading to an application using Delphi's TThread Class. The thread calls a function which compares two files and print the bits that are different between them. Before I introduced threading the application could complete this procedure and print the output in about 1 - 2 seconds on a 300KB file. After introduction of threading checking the same file can take up to 30 - 45 seconds and cause a 50% CPU spike (AMD Phenom II Triple Core), previously you didn't notice a spike.

The code that is being executed by the thread is:

procedure TForm1.CompareFiles(fil1, fil2 : ansistring; sg : TStringGrid; option : integer; progb : TProgressBar);
var
forg, fpat : file;
byteorg, bytepat : Byte;
byteorgc,bytepatc : ansistring;
arrby : Array Of ansistring;
arrpos : Array Of ansistring;
i,x : integer;
begin

if CRCAdlerGenFile(fil1,1) <> CRCAdlerGenFile(fil2,1) then //Only Run if files arn't same
begin
sg.Cols[0].Clear;
sg.Cols[1].Clear;
i := 0;
x := 0;

AssignFile(forg,fil1);
FileMode := fmOpenRead;
Reset(forg,1);
AssignFile(fpat,fil2);
FileMode := fmOpenRead;
Reset(fpat,1);

//Set Progress Bar
progb.Min := 0;
progb.Max := FileSize(forg);

while NOT eof(forg) do
begin
BlockRead(forg,byteorg,1);
BlockRead(fpat,bytepat,1);
Progb.Position := Progb.Position + 1;
byteorgc := IntToHex(byteorg,2);
bytepatc := IntToHex(bytepat,2);
if byteorg <> bytepat then
begin
x := x + 1;
SetLength(arrby,x);
SetLength(arrpos,x);
arrpos[i] := IntToStr(FilePos(forg));
arrby[i] := bytepatc;
i := i + 1;
end;
end;

CloseFile(forg);
CloseFile(fpat);


case option of
0 : begin //Base 2
    for I := 0 to (Length(arrpos) - 1) do
    begin
    arrpos[i] := IntToBin(StrToInt(arrpos[i]),8);
    end;
    end;

1 : ; //Base 10

2 :  begin //Base 16
    for I := 0 to (Length(arrpos) - 1) do
      begin
        arrpos[i] := IntToHex(StrToInt(arrpos[i]),1);
      end;
    end;

3 : begin //Append $
    for I := 0 to (Length(arrpos) - 1) do
    begin
    arrpos[i] := '$'+IntToHex(StrToInt(arrpos[i]),1);
    end;
    end;

4 : begin //Append 0x
    for I := 0 to (Length(arrpos) - 1) do
    begin
    arrpos[i] := '0x'+IntToHex(StrToInt(arrpos[i]),1);
    end;
    end;
end;


Sg.RowCount := Length(arrpos);
for I := 0 to (Length(arrpos) - 1) do
begin
  sg.Cells[0,i] := arrpos[i];
  sg.Cells[1,i] := arrby[i];
end;

if sg.RowCount >= 16 then
sg.DefaultColWidth := 222
else
sg.DefaultColWidth := 231;
end;

end;

The threading code used was pretty much taken from this previous question I asked with slight name changes and the introduction and a progress bar variable, however that was added after I noticed the slow processing as a way for me to monitor it.

Link to previous question for threading code.

UPDATE:

Fixed Code Looks something like this. I have totally remove the function CompareFiles and moved the code into Thread.Execute for ease of read/usage.

 procedure TCompareFilesThread.Execute;
  var
forg, fpat : file;
byteorg, bytepat : Array[0..1023] of byte;
i,z,o : integer;
fil1,fil2 : TFilename;
begin
 //Form1.CompareFiles(FEdit3Text, FEdit4Text, FGrid, FOp, FProg);

begin
  fil1 := Form1.Edit3.Text;
  fil2 := Form1.Edit4.Text;
if Form1.CRCAdlerGenFile(fil1,1) <> Form1.CRCAdlerGenFile(fil2,1) then //Only Run if files arn't same
begin

i := 0;
x := 1;
o := 0;

AssignFile(forg,fil1);
FileMode := fmOpenRead;
Reset(forg,1);
AssignFile(fpat,fil2);
FileMode := fmOpenRead;
Reset(fpat,1);

//Set Progress Bar

while NOT eof(forg) do
begin
    while Terminated = False do
      begin
        BlockRead(forg,byteorg,1024);
        BlockRead(fpat,bytepat,1024);

        for z := 0 to 1023 do
          begin
            if byteorg[z] <> bytepat[z] then
            begin
            synchronize(sProgBarNext);
            by := bytepat[z];
            off := IntToStr(o);
            synchronize(SyncGrid);
            inc(x);
          end;
        inc(o);
        end;
    end;
end;

CloseFile(forg);
CloseFile(fpat);
end;
end;
Free;
end;

Sync Grid

procedure TCompareFilesThread.SyncGrid;
begin
  form1.StringGrid2.RowCount := x;

    if x >= 16 then
      form1.StringGrid2.DefaultColWidth := 222
    else
      Form1.StringGrid2.DefaultColWidth := 232;

        case op of
          0 : off := IntToBin(StrToInt(off),8);    //Base 2
          1 : ; //Base 10
          2 : off := IntToHex(StrToInt(off),1);//Base 16
          3 : off := '$'+IntToHex(StrToInt(off),1); //Append $
          4 : off := '0x'+IntToHex(StrToInt(off),1);//Append 0x
        end;

  form1.StringGrid2.Cells[0,(x-1)] := off;
  form1.StringGrid2.Cells[1,(x-1)] := IntToHex(by,2);
end;

Sync Prog

procedure TCompareFilesThread.SProgBarNext;
begin
Form1.ProgressBar1.Position := Form1.ProgressBar1.Position + 1;
end;

I'm somewhat surprised that it runs at all. But again, youve obviously changed the code a lot since your linked previous question, making it hard to guess at what is actually happening. The 50% CPU spike makes me suspect you have at least two cores, and it would be handy to know that. Please post your whole program. — , Jan 27 '11 at 22:51
In my answer to your previous question, I wrote this: "One last thing to beware of, though, is that `TStringGrid` is a VCL control. You mustn't do anything with it from this new thread you create (regardless of how you end up creating it). Eveything you do with the grid control need to be done from the main thread. Use `TThread.Synchronize` and `TThread.Queue` to shift any VCL operations onto the main thread." I don't see you heeding that advice. — Rob Kennedy, Jan 27 '11 at 23:18
@Rob Kennedy Apologies. I had read that part just obviously never took it in. @moz I do indeed have three cores and have updated the question to state that. I haven't added the full program as this is only a part of a much larger program that does many other unrelated things. Also why are you surprised it runs at all, since the code hasn't been changed since my previous question with the exception of the `TThread` code from it and a progress bar. — Flatlyn, Jan 27 '11 at 23:27
The way you're accessing the file is indeed a source of bad performance: I advice you that instead of **reading single byte chunks** for both files you can read large chunks (4K or more) and then compare byte per byte if you want, and, **don't convert the bytes to strings** to perform comparison... just compare the bytes!! — jachguate, Jan 27 '11 at 23:59
Flat, the reason I'm surprised it runs at all is the interaction with the VCL and especially the string grid. But as you've been told before, read the links posted and think about how those relate to your situation. Unless you can do that we can't help you. — , Jan 28 '11 at 02:17
@jachguate I've already mostly coded an alternative version which loads more of the file (5KB) into an array and then does the compare. The reason I'm working off this just now is that is the process in it's most basic form which I tend to find it easier for debugging and since I haven't got it working right yet don't see the point in optimizing. Although that is a source of low performance it shouldn't be the source of the performance change while using `TThread` @moz It was my mistake for not reading Rob's reply fully, busy updating my code now. — Flatlyn, Jan 28 '11 at 02:19
@TheFlatline: 5kB? Your files are most likely stored on disk in 4kb blocks. Optimally you should read multiples of 4KB: if you don't like 4KB, use 8KB or 16KB. — Cosmin Prund, Jan 28 '11 at 12:54
@TheFlatline: Reading a file in chunks is a kind of required basic coding I don't like to call _optimization_. Reading a file one byte at once require almost the same coding effort. — jachguate, Jan 28 '11 at 20:54

score 14 · Accepted Answer · answered Jan 27 '11 at 22:32

This code is running in a different thread? Well, one obvious problem is its use of VCL controls. The VCL is not threadsafe, and attempting to update VCL properties from outside the main thread is bound to cause problems. This needs to be refactored pretty heavily. The point of your threaded routine is to perform calculations. You should not be passing in a TStringGrid, and you shouldn't be updating progress bars.

Take a look at the Synchronize and Queue methods of the TThread class for the correct ways to interact with the main thread from a worker thread. It'll take a bit of work, but what you'll end up will be faster and cleaner.

score 0 · Answer 2 · answered Jan 29 '11 at 21:02

0

Default thread priority in Delphi is tpLower which might account for the fact that it runs slower than you expect. Others have correctly pointed out that this piece of code is terribly dangerous. Don't even consider updating a UI control from a secondary thread in Delphi.

answered Jan 29 '11 at 21:02

Turin

129
2
10

2

Could you explain how a lower thread priority on a system with spare CPU cycles could lead to a performance drop by much more than 90%? – mghie Jan 29 '11 at 22:23

score 0 · Answer 3 · answered Nov 11 '22 at 15:29

Note that synchronize(SyncGrid) stops the thread while waiting main thread to execute SyncGrid and resumes when finished. TThread.Queue() queues the method and does not stop the thread, but in your case it won't work well, since SyncGrid has a lot more work than thread.execute.

Since most of work is updating GUI(grid and progressbar), you do not benefit from processing in a separate thread.

Also, this isn't thread safe:

`fil1 := Form1.Edit3.Text;`
`fil2 := Form1.Edit4.Text;`

Instead, pass these two values as params in thread constructor.

TCompareFilesThread = class(TThread)
  fil1,fil2 : string;
  constructor Create(Afil1,Afil2 : string);
....
AThread := TCompareFilesThread.Create(Edit3.Text,Edit4.Text);

This code might not be safe either:

if Form1.CRCAdlerGenFile(fil1,1) <> ....

If CRCAdlerGenFile() uses no other stuff from Form1, is safe to use from thread, but no point of being Form1 method.

Why does my code run slower if I call it from a separate thread?

3 Answers3