views:

262

answers:

7

Hello !

i have a simple program with one procedure.

Procedure TForm1.btnKeywrdTransClick(Sender: TObject);
Var
  i, ii             : integer;
  ch_word, zword, uy_word: widestring;
Begin
  TntListBox1.items.LoadFromFile('d:\new folder\chh.txt'); //Chinese 
  TntListBox2.items.LoadFromFile('d:\new folder\uyy.txt'); //Uyword
  TntListBox4.items.LoadFromFile(Edit3.text); //list of poi files
  For I := 0 To TntListBox4.items.Count - 1 do
  Begin
  TntListBox3.items.LoadFromFile(TntListBox4.Items[i]);
  zword := tntlistbox3.Items.Text;      //Poi
  For ii := 0 To TntListBox1.Items.count - 1 Do
  Begin
    loopz;
    ch_word := tntlistbox1.Items[ii];
    uy_word := ' ' + TntListBox2.items[ii] + ' ';
    zword := wideFastReplace(zword, ch_word, uy_word, [rfReplaceAll]); //fastest, and better for large text
  End;
  TntListBox3.Items.text := zword;
  TntListBox3.items.SaveToFile(TntListBox4.Items[i]);
end;
end;

now my new computer has 4cores, is making this program multithreading will make it run faster (if i uses 4 thread, a thread per core) ? i have no experience with multithreading, i need your help thanks.

ps : this is Loopz procedure

Procedure loopz;
Var
  msg               : tmsg;
Begin
  While PeekMessage(Msg, 0, 0, 0, pm_Remove) Do
  Begin
    If Msg.Message = wm_Quit Then Halt(Msg.WParam);
    TranslateMessage(Msg);
    DispatchMessage(Msg);
  End;
End;

update 1 : from the answers, im gonna do

1 - use a profiler to find the most time consuming code

2 - try eliminate gui related things if possible

3 - use threads.

i'll report back . thanks all.

+2  A: 

It looks like you are interfacing with GUI elements.

99% of all GUI code must be interfaced from one and only one thread.

If you refactor your code to perform the text replacements in a series of threads, dividing the text amongst them, and then have the GUI thread place it into your list box, you could improve performance.

Note that creating and synchronizing threads is not cheap. Unless you have thousands of entries to work on, you will likely slow down your program by adding threads.

Yann Ramin
A: 

if you use Delphi Prism then you could take advantage of the parallel task library in .NET 4.0

Sebastian Godelet
Please, when downvoting something tell us why!
Remko
stackoverflow users are sometimes very narrow-minded i feel ...
Sebastian Godelet
i have made very good experience with Delphi on .NET. you can gain soo much functionality for free (like to solve this problem effectively)
Sebastian Godelet
Well, I didn't downvote, but I als wouldn't upvote. I doubt that the OP expected (nor will accept) any answer that suggests to use a different programming environment.
Uwe Raabe
Frikkin downvoters! It may not be a good answer for the OP as @Uwe Raabe pointed out, but it is not stupid either. And I find it rude to downvote blindly without giving the poster a chance to understand why! After all, if you think he/she needs to improve, provide some help! Thanks @Uwe for doing the right thing.
François
+4  A: 

First you should profile your code to see if reading from TntListBox is slowing you down or if it is WideFastReplace. But even before that, remove the 'loopz' call - it is slowing you the most! Why are you processing messages inside this loop at all?

To find the bottleneck, simply time your loop twice, but the second time comment out the WideFastReplace call. (And make sure you are timing only the loop, not the assignment to the TntListBox3 or saving into file or loading from file.)

When you will know what's slowing you down, report back ...

BTW, calling WideFastReplace in parallel would be almost impossible as it is always operating on the same source. I don't see any good way to parallelize your code.

A possible parallelization approach:

  • Split zword on an appropriate word delimiter (I'm assuming here you are only replacing words, not phrases) into N strings where N is the number of cores.
  • Do the full replacement (all search/replacement pairs) for each of those N strings in parallel. Of course, you would have to read search/replacement pairs first from the TntListBoxes into some internal structure (TStringList would suffice) and then use this structure in all N threads.
  • Concatenate those partial strings back together.

Of course, there's no point in doing that if WideFastReplace is not the time-consuming part of the code. Do the profiling first!

gabr
There's a easy way to parallelize the code, look at my answer. The biggest problem is giving the lists proper names so everyone sees and understands what the algorithm is actually doing!
Cosmin Prund
@Cosmin: The code does zword := replace(zword, a, b, [all]) and there's no easy way to parallelize that.
gabr
@gabr You don't parallelize that; You parallelize the first loop (the one going over the filenames).
Cosmin Prund
@Cosmin: Ah, yes, that one can be simply parallelized.
gabr
hi gabr, i used wideFastReplace after testing few string replacement procedures. including the one from delphi rtl, this one was the fastest one . (i converted it from faststring unit to work with unicode)the weird thing is, smaller the the zword is slower the result. so i pass it whole file .
avar
+1  A: 

You should gain quite a bit of improvement by using only one thread for the whole thing. With this you can omit the loopz call completely.

Be aware that you should replace the TntListboxes with local TWideStringList instances in your routine.

When you have gotten somewhat familiar with multithreading, you can go and split the work into multiple threads. This can be done for instance by splitting the list of poi files (listbox4) in multiple (say 3-4) lists, one for each thread.

Uwe Raabe
+13  A: 

First of all make the algorithm as effective as it can be in it's current incarnation: Stop using TListBox to store your data!!! (sorry for shouting) Replace them with TStringList and you'll get a HUGE performance improvement. That's an required first step any way, because you can't use GUI objects from multiple threads (in fact you may only use them from the "main" thread). While you're changing TListBox to TStringList please give your variable meaningful names. I don't know how many people around here figured out that you're storing a list of file names in ListBox4, loading each file in ListBox3, using ListBox1 as a "keyword list" and ListBox2 as a "value list"... really, it's a big mess! Here's how it would look like with TStringList and proper names:

Procedure TForm1.btnKeywrdTransClick(Sender: TObject);
Var
  i, ii             : integer;
  ch_word, zword, uy_word: widestring;
  PoiFilesList:TStringList; // This is the list of files that need work
  PoiFile:TStringList; // This is the file I'm working on right now
  KeywordList, ValueList:TStringList; // I'll replace all keywords with corresponding values
Begin

  PoiFilesList := TStringList.Create;
  PoiFile := TStringList.Create;
  KeywordList := TStringList.Create;
  ValueList := TStringList.Create;

  try
    PoiFilesList.LoadFromFile(Edit3.text); //list of poi files
    KeywordList.LoadFromFile('d:\new folder\chh.txt'); //Chinese 
    ValueList.LoadFromFile('d:\new folder\uyy.txt'); //Uyword
    For I := 0 To PoiFilesList.Count - 1 do
    Begin
      PoiFile.LoadFromFile(PoiFilesList[i]);
      zword := PoiFile.Text;      //Poi
      For ii := 0 To KeywordList.count - 1 Do
      Begin
        ch_word := KeywordList[ii];
        uy_word := ' ' + ValueList[ii] + ' ';
        zword := wideFastReplace(zword, ch_word, uy_word, [rfReplaceAll]);
      End;
      PoiFile.text := zword;
      PoiFile.SaveToFile(PoiFilesList[i]);
    end;
  finally
    PoiFilesList.Free;
    PoiFile.Free;
    KeywordList.Free;
    ValueList.Free;
  end;
end;

If you look at the code now, it's obvious what it does, and it's obvious how to multi-thread-it. You've got a text file containing names of files. You open up each one of those files and replace all Keywords with the corresponding Values. You save the file back to disk. It's easy! Load the KeywordList and ValueList to memory once, split the list of files into 4 smaller lists, start up 4 threads each working with it's own smaller files list.

I don't want to writhe the whole multi-threaded variant of the code because if I'll write it myself you might not understand how it works. Give it a chance and ask for help if you get into trouble.

Cosmin Prund
+1 for the TStringList; I wish I could add more + for the other good ideas (naming, try..finally).
Jeroen Pluimers
hi,i already have TWideStringList ( unicode.pas from Mike Lischke) version of this code. and yes it is faster.but i need to watch the result live, so i'm forced to use listbox ;)
avar
another thing, i don't alter strings in listbox directly, i move the strings to zword to get some speed.
avar
@avar: "Watch the results live" / "Do it as fast as possible" don't go well together. In fact, if this code is running as fast as it should all you'd see is flickering gray boxes. If you want some sort of graphic progress information, use a TProgressBar!
Cosmin Prund
progress bar cannot show me the result after replacement :) how the new word looks like ?
avar
@avar: You can see the translated texts in the corresponding text files. You started a question asking how to "make it run faster" using multiple threads on multi-core CPU, now you're saying you don't want it all that fast because you need to "read the words live". How fast can you read? Again, if this is going at full speed, text should be changing so fast all you'd see is a gray box! If you don't actually want it that fast, you don't want it multi-threaded any way.
Cosmin Prund
Having multiple threads all doing I/O is bad advise, most of the time. The reason being that unless the disc is an SSD the concurrent requests for I/O on different disc areas may actually make things *much* slower than using a single thread for I/O, due to the additional disc seek operations. I/O should be confined to a single thread instead, and one (or more) other threads should do the text replacement. Look into producer-consumer queues as the starting point.
mghie
To watch the results live AND do it as fast as possible, use a timer, use the stringlist, and every 0.1 seconds or so, update your listbox.
lkessler
+1  A: 

Operations that could be run in parallel benefit from multitasking - those that have to be run one after another can't. The larger the operation, the larger the benefit. In your procedure you could parallelize the file loadings (although I guess they hold not so many elements) and you could parallelize the replace operation having multiple threads operating each on different list elements. How much faster it will run depends of the files size. I guess you have more speed penality in using GUI elements to store data instead of working directly on in-memory structure, because you that means redrawing the controls often, which is an expensive operation.

ldsandon
A: 

Here is your answer 1. If you can, do not wait until user click to react to the action. Do it before hand like on formcreate by Put them into wrapper object Run it under a thread; once finish, mark it to be ready to be used When user click on the action, check for marker. If it is not done
yet do a while loop and wait something like

btnKeywrdTrans.Enabled := False;
while not wrapper.done do
begin
  Sleep(500);
  Application.Processmessages;
end; 
..... your further logic
btnKeywrdTrans.Enabled := True;
  1. Replace it with TStringList or TWideStringList

Cheers Pham

APZ28