5

I have a time consuming routine which I'd like to process in parallel using Delphi XE7's new parallel library.

Here is the single threaded version:

procedure TTerritoryList.SetUpdating(const Value: boolean);
var
  i, n: Integer;
begin
  if (fUpdating <> Value) or not Value then
  begin
    fUpdating := Value;

    for i := 0 to Count - 1 do
    begin
      Territory[i].Updating := Value; // <<<<<< Time consuming routine
      if assigned(fOnCreateShapesProgress) then
        fOnCreateShapesProgress(Self, 'Reconfiguring ' + Territory[i].Name, i / (Count - 1));
    end;
  end;
end;

There is really nothing complex going on. If the territory list variable is changed or set to false then the routine loops around all the sales territories and recreates the territory border (which is the time consuming task).

So here is my attempt to make it parallel:

procedure TTerritoryList.SetUpdating(const Value: boolean);
var
  i, n: Integer;
begin
  if (fUpdating <> Value) or not Value then
  begin
    fUpdating := Value;

    n := Count;
    i := 0;

    TParallel.For(0, Count - 1,
      procedure(Index: integer)
      begin
        Territory[Index].Updating := fUpdating; // <<<<<< Time consuming routine
        TInterlocked.Increment(i);
        TThread.Queue(TThread.CurrentThread,
          procedure
            begin
              if assigned(fOnCreateShapesProgress) then
                fOnCreateShapesProgress(nil, 'Reconfiguring ', i / n);
            end);
      end
    );
  end;
end;

I've replaced the for-loop with a parallel for-loop. The counter, 'i' is locked as it is incremented to show progress. I then wrap the OnCreateShapeProgress event in a TThread.Queue, which will be handled by the main thread. The OnCreateShapeProgress event is handled by a routine which updates the progress bar and label describing the task.

The routine works if I exclude the call to the OnCreateShapeProgress event. It crashes with an EAurgumentOutOfRange error.

So my question is simple:

Am I doing anything anything stupid?

How to do you call an event handler from within a TParallel.For loop or TTask?

Steve Maughan
  • 1,174
  • 3
  • 19
  • 30
  • `TParallel.For` has a bug in XE7, that is fixed in update 1. I don't think it has any significance here, but test with the update anyway. See [TParallel.For reserves, but not frees memory](https://quality.embarcadero.com/browse/RSP-9564). – LU RD Nov 14 '14 at 22:57

1 Answers1

4

The most obvious problem that I can see is that you queue to the worker thread.

Your call to TThread.Queue passes TThread.CurrentThread. That is the very thread on which you are calling TThread.Queue. I think it is safe to say that you should never pass TThread.CurrentThread to TThread.Queue.

Instead, remove that parameter. Use the one parameter overload that just accepts a thread procedure.

Otherwise I'd note that the incrementing of the progress counter i is not really handled correctly. Well, the incrementing is fine, but you then read it later and that's a race. You can report progress out of order if thread 1 increments before thread 2 but thread 2 queues progress before thread 1. Solve that by moving the counter increment code to the main thread. Simply increment it inside the queued anonymous method. Added bonus to that is you no longer need to use an atomic increment since all modifications are on the main thread.

Beyond that, this QC report seems rather similar to what you report: http://qc.embarcadero.com/wc/qcmain.aspx?d=128392

Finally, AtomicIncrement is the idiomatic way to perform lock free incrementing in the latest versions of Delphi.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • Thanks David - this was helpful. Passing `TThread.CurrentThread` is what Danny Wind did in his recent Coderage 9 talk (at 21 mins 25 secs). I agree it seems incorrect. When I change it to `nil` it doesn't crash but it doesn't show the progress bar. When I change the `TThread.Queue` to `TThread.Syncronize` it gives an `EArgumentOutOfRange` error. – Steve Maughan Nov 15 '14 at 03:16
  • It looks as if when `TThread,Queue` is used the code is only executed after the routine has finished (which sort of makes sense). So changing to `TThread.Syncronize` should solve this problem, but I still get the error. – Steve Maughan Nov 15 '14 at 03:32
  • You should use the one parameter overload of Queue. – David Heffernan Nov 15 '14 at 08:12
  • 1
    Perhaps it is a Delphi bug. See http://qc.embarcadero.com/wc/qcmain.aspx?d=128392 Frankly I wouldn't trust the Emba rtl devs to put together a threading library. They don't have a good track record in this field. – David Heffernan Nov 15 '14 at 08:35
  • @SteveMaughan, Update 1 for XE7 is out. The QC seems to be fixed there. Prease install the update, recompile and rerun your project. – iPath ツ Nov 15 '14 at 13:34
  • @iPathツ - I have upgraded. The `EAugumentOutOfRange` error has gone! Thanks. – Steve Maughan Nov 17 '14 at 15:29
  • The comments here seem to indicate that TThread.CurrentThread should NOT be used because it is the "main" thread - however, this is NOT the case - TThread.Current thread is ACTUALLY the Parallel Task Thread, as the implementation uses TParallel.For - which creates a thread for the procedure (Index) to execute on. - The "paralled" threads will then get "paused" until the application goes idle at which point, the CheckSynchronize gets called. The Event will be executed by the main thread. The code posted above is actually correct and "safe" for the Queue – SilverKnight Feb 02 '15 at 19:39
  • @SilverKnight I state that TThread.CurrentThread is the worker thread. And not the main thread. The point of this call to Queue is to invoke the method on the UI thread, that well known idiom. – David Heffernan Feb 02 '15 at 20:01