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

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
  开发者_Go百科  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;

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.

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.

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.