LZMACompressor: Report progress asynchronously.

Couple of improvements here:



1. Previously, ProgressProc was called whenever the LZMA encoder decided to report progress. This usually worked OK, but with some settings/data, there could be long delays between each progress report, e.g. half a second or more, and this made the IDE's UI slow to respond. Now, instead of waiting on progress reports, the main thread wakes itself up at a fixed interval to check for progress changes.



2. When progress was being reported, the worker thread previously waited until the main thread finished handling the event. (I can't recall exactly why it did that; it may have been an attempt to improve UI responsiveness on single-core systems by effectively forcing context switches.) This wait has been eliminated, allowing the main thread to update its UI while compression continues concurrently.

Projects/Src/Compression.LZMACompressor.pas

@@ -67,16 +67,14 @@ type
     StartEncodeEvent: THandle;
     EndWaitOnInputEvent: THandle;
     EndWaitOnOutputEvent: THandle;
-    EndWaitOnProgressEvent: THandle;
     WorkerWaitingOnInputEvent: THandle;
     WorkerWaitingOnOutputEvent: THandle;
-    WorkerHasProgressEvent: THandle;
     WorkerEncodeFinishedEvent: THandle;
   end;
   PLZMACompressorSharedData = ^TLZMACompressorSharedData;
   TLZMACompressorSharedData = record
+    ProgressBytes: Int64;
     NoMoreInput: BOOL;
-    ProgressKB: UInt32;
     EncodeResult: TLZMASRes;
     InputBuffer: TLZMACompressorRingBuffer;
     OutputBuffer: TLZMACompressorRingBuffer;
@@ -101,11 +99,13 @@ type
     FEncodeStarted: Boolean;
     FEncodeFinished: Boolean;
     FLastInputWriteCount: LongWord;
-    FLastProgressKB: UInt32;
+    FLastProgressBytes: Int64;
+    FProgressTimer: THandle;
+    FProgressTimerSignaled: Boolean;
     procedure FlushOutputBuffer(const OnlyOptimalSize: Boolean);
     procedure InitializeProps(const CompressionLevel: Integer;
       const ACompressorProps: TCompressorProps);
-    class function IsEventSet(const AEvent: THandle): Boolean;
+    class function IsObjectSignaled(const AObject: THandle): Boolean;
     class procedure SatisfyWorkerWait(const AWorkerEvent, AMainEvent: THandle);
     procedure SatisfyWorkerWaitOnInput;
     procedure SatisfyWorkerWaitOnOutput;
@@ -145,7 +145,7 @@ type
 implementation
 
 const
-  ISLZMA_EXE_VERSION = 101;
+  ISLZMA_EXE_VERSION = 102;
 
 type
   TLZMACompressorHandle = type Pointer;
@@ -155,7 +155,7 @@ type
     FThread: THandle;
     FLZMAHandle: TLZMACompressorHandle;
     FReadLock, FWriteLock, FProgressLock: Integer;
-    FLastProgressTick: DWORD;
+    function CheckTerminateWorkerEvent: HRESULT;
     function FillBuffer(const AWrite: Boolean; const Data: Pointer;
       Size: Cardinal; var ProcessedSize: Cardinal): HRESULT;
     function ProgressMade(const TotalBytesProcessed: UInt64): HRESULT;
@@ -539,6 +539,17 @@ begin
   end;
 end;
 
+function TLZMAWorkerThread.CheckTerminateWorkerEvent: HRESULT;
+begin
+  case WaitForSingleObject(FEvents.TerminateWorkerEvent, 0) of
+    WAIT_OBJECT_0 + 0: Result := E_ABORT;
+    WAIT_TIMEOUT: Result := S_OK;
+  else
+    SetEvent(FEvents.TerminateWorkerEvent);
+    Result := E_FAIL;
+  end;
+end;
+
 function TLZMAWorkerThread.FillBuffer(const AWrite: Boolean;
   const Data: Pointer; Size: Cardinal; var ProcessedSize: Cardinal): HRESULT;
 { Called from worker thread (or a thread spawned by the worker thread) }
@@ -620,28 +631,20 @@ end;
 
 function TLZMAWorkerThread.ProgressMade(const TotalBytesProcessed: UInt64): HRESULT;
 { Called from worker thread (or a thread spawned by the worker thread) }
-var
-  T: DWORD;
 begin
-  T := GetTickCount;
-  if Cardinal(T - FLastProgressTick) >= Cardinal(100) then begin
-    { Sanity check: Make sure we're the only thread inside Progress }
-    if InterlockedExchange(FProgressLock, 1) <> 0 then begin
-      Result := E_FAIL;
-      Exit;
-    end;
-    FLastProgressTick := T;
-    { Make sure TotalBytesProcessed isn't negative. LZMA's Types.h says
-      "-1 for size means unknown value", though I don't see any place
-      where LzmaEnc actually does call Progress with inSize = -1. }
-    if Int64(TotalBytesProcessed) >= 0 then
-      FShared.ProgressKB := UInt32(TotalBytesProcessed shr 10);
-    Result := WakeMainAndWaitUntil(FEvents.WorkerHasProgressEvent,
-      FEvents.EndWaitOnProgressEvent);
-    InterlockedExchange(FProgressLock, 0);
-  end
-  else
-    Result := S_OK;
+  { Sanity check: Make sure we're the only thread inside Progress }
+  if InterlockedExchange(FProgressLock, 1) <> 0 then begin
+    Result := E_FAIL;
+    Exit;
+  end;
+  { An Interlocked function is used to ensure the 64-bit value is written
+    atomically (not with two separate 32-bit writes).
+    TLZMACompressor will ignore negative values. LZMA SDK's 7zTypes.h says
+    "-1 for size means unknown value", though I don't see any place
+    where LzmaEnc actually does call Progress with inSize = -1. }
+  InterlockedExchange64(FShared.ProgressBytes, Int64(TotalBytesProcessed));
+  Result := CheckTerminateWorkerEvent;
+  InterlockedExchange(FProgressLock, 0);
 end;
 
 { TLZMAWorkerProcess }
@@ -712,10 +715,8 @@ procedure TLZMAWorkerProcess.SetProps(const LZMA2: Boolean;
     DupeEvent(Src.StartEncodeEvent, Dest.StartEncodeEvent);
     DupeEvent(Src.EndWaitOnInputEvent, Dest.EndWaitOnInputEvent);
     DupeEvent(Src.EndWaitOnOutputEvent, Dest.EndWaitOnOutputEvent);
-    DupeEvent(Src.EndWaitOnProgressEvent, Dest.EndWaitOnProgressEvent);
     DupeEvent(Src.WorkerWaitingOnInputEvent, Dest.WorkerWaitingOnInputEvent);
     DupeEvent(Src.WorkerWaitingOnOutputEvent, Dest.WorkerWaitingOnOutputEvent);
-    DupeEvent(Src.WorkerHasProgressEvent, Dest.WorkerHasProgressEvent);
     DupeEvent(Src.WorkerEncodeFinishedEvent, Dest.WorkerEncodeFinishedEvent);
   end;
 
@@ -805,11 +806,12 @@ begin
   FEvents.StartEncodeEvent := LZMACreateEvent(False);          { auto reset }
   FEvents.EndWaitOnInputEvent := LZMACreateEvent(False);       { auto reset }
   FEvents.EndWaitOnOutputEvent := LZMACreateEvent(False);      { auto reset }
-  FEvents.EndWaitOnProgressEvent := LZMACreateEvent(False);    { auto reset }
   FEvents.WorkerWaitingOnInputEvent := LZMACreateEvent(True);  { manual reset }
   FEvents.WorkerWaitingOnOutputEvent := LZMACreateEvent(True); { manual reset }
-  FEvents.WorkerHasProgressEvent := LZMACreateEvent(True);     { manual reset }
   FEvents.WorkerEncodeFinishedEvent := LZMACreateEvent(True);  { manual reset }
+  FProgressTimer := CreateWaitableTimer(nil, False, nil);      { auto reset }
+  if FProgressTimer = 0 then
+    LZMAWin32Error('CreateWaitableTimer');
   InitializeProps(CompressionLevel, ACompressorProps);
 end;
 
@@ -823,11 +825,10 @@ destructor TLZMACompressor.Destroy;
 
 begin
   FWorker.Free;
+  DestroyEvent(FProgressTimer);
   DestroyEvent(FEvents.WorkerEncodeFinishedEvent);
-  DestroyEvent(FEvents.WorkerHasProgressEvent);
   DestroyEvent(FEvents.WorkerWaitingOnOutputEvent);
   DestroyEvent(FEvents.WorkerWaitingOnInputEvent);
-  DestroyEvent(FEvents.EndWaitOnProgressEvent);
   DestroyEvent(FEvents.EndWaitOnOutputEvent);
   DestroyEvent(FEvents.EndWaitOnInputEvent);
   DestroyEvent(FEvents.StartEncodeEvent);
@@ -892,21 +893,21 @@ begin
   FWorker.SetProps(FUseLZMA2, EncProps);
 end;
 
-class function TLZMACompressor.IsEventSet(const AEvent: THandle): Boolean;
+class function TLZMACompressor.IsObjectSignaled(const AObject: THandle): Boolean;
 begin
   Result := False;
-  case WaitForSingleObject(AEvent, 0) of
+  case WaitForSingleObject(AObject, 0) of
     WAIT_OBJECT_0: Result := True;
     WAIT_TIMEOUT: ;
   else
-    LZMAInternalError('IsEventSet: WaitForSingleObject failed');
+    LZMAInternalError('IsObjectSignaled: WaitForSingleObject failed');
   end;
 end;
 
 class procedure TLZMACompressor.SatisfyWorkerWait(const AWorkerEvent,
   AMainEvent: THandle);
 begin
-  if IsEventSet(AWorkerEvent) then begin
+  if IsObjectSignaled(AWorkerEvent) then begin
     if not ResetEvent(AWorkerEvent) then
       LZMAWin32Error('SatisfyWorkerWait: ResetEvent');
     if not SetEvent(AMainEvent) then
@@ -928,15 +929,30 @@ procedure TLZMACompressor.UpdateProgress;
 const
   MaxBytesPerProgressProcCall = 1 shl 30;  { 1 GB }
 var
-  NewProgressKB: UInt32;
-  Bytes: UInt64;
+  NewProgressBytes, Bytes: Int64;
   LimitedBytes: Cardinal;
 begin
-  if IsEventSet(FEvents.WorkerHasProgressEvent) then begin
+  { Check if the timer is signaled. Because it's an auto-reset timer, this
+    also resets it to non-signaled. Note that WaitForWorkerEvent also waits
+    on the timer and sets FProgressTimerSignaled. }
+  if IsObjectSignaled(FProgressTimer) then
+    FProgressTimerSignaled := True;
+
+  if FProgressTimerSignaled then begin
+    FProgressTimerSignaled := False;
     if Assigned(ProgressProc) then begin
-      NewProgressKB := FShared.ProgressKB;
-      Bytes := UInt64(UInt32(NewProgressKB - FLastProgressKB)) shl 10;  { wraparound is OK }
-      FLastProgressKB := NewProgressKB;
+      { An Interlocked function is used to ensure the 64-bit value is read
+        atomically (not with two separate 32-bit reads). }
+      NewProgressBytes := InterlockedExchangeAdd64(FShared.ProgressBytes, 0);
+
+      { Make sure the new value isn't negative or going backwards. A call
+        to ProgressProc is always made, even if the byte count is 0. }
+      if NewProgressBytes > FLastProgressBytes then begin
+        Bytes := NewProgressBytes - FLastProgressBytes;
+        FLastProgressBytes := NewProgressBytes;
+      end else
+        Bytes := 0;
+
       repeat
         if Bytes >= MaxBytesPerProgressProcCall then
           LimitedBytes := MaxBytesPerProgressProcCall
@@ -946,10 +962,6 @@ begin
         Dec(Bytes, LimitedBytes);
       until Bytes = 0;
     end;
-    if not ResetEvent(FEvents.WorkerHasProgressEvent) then
-      LZMAWin32Error('UpdateProgress: ResetEvent');
-    if not SetEvent(FEvents.EndWaitOnProgressEvent) then
-      LZMAWin32Error('UpdateProgress: SetEvent');
   end;
 end;
 
@@ -981,19 +993,39 @@ begin
 end;
 
 procedure TLZMACompressor.StartEncode;
+
+  procedure StartProgressTimer;
+  const
+    { This interval was chosen because:
+      - It's two system timer ticks, rounded up:
+          (1000 / 64) * 2 = 31.25
+      - The keyboard repeat rate is 30/s by default:
+          1000 / 30 = 33.333
+        So if an edit control is focused and the ProgressProc is processing
+        messages, the caret should move at full speed when an arrow key is
+        held down. }
+    Interval = 32;
+  begin
+    FProgressTimerSignaled := False;
+    var DueTime := Int64(-10000) * Interval;
+    if not SetWaitableTimer(FProgressTimer, DueTime, Interval, nil, nil, False) then
+      LZMAWin32Error('SetWaitableTimer');
+  end;
+
 begin
   if not FEncodeStarted then begin
     FShared.NoMoreInput := False;
-    FShared.ProgressKB := 0;
+    FShared.ProgressBytes := 0;
     FShared.EncodeResult := -1;
     RingBufferReset(FShared.InputBuffer);
     RingBufferReset(FShared.OutputBuffer);
     FLastInputWriteCount := 0;
-    FLastProgressKB := 0;
+    FLastProgressBytes := 0;
     FEncodeFinished := False;
     FEncodeStarted := True;
     if not ResetEvent(FEvents.WorkerEncodeFinishedEvent) then
       LZMAWin32Error('StartEncode: ResetEvent');
+    StartProgressTimer;
     if not SetEvent(FEvents.StartEncodeEvent) then
       LZMAWin32Error('StartEncode: SetEvent');
   end;
@@ -1014,13 +1046,13 @@ begin
     events. }
   H[0] := FWorker.GetExitHandle;
   H[1] := FEvents.WorkerEncodeFinishedEvent;
-  H[2] := FEvents.WorkerHasProgressEvent;
+  H[2] := FProgressTimer;
   H[3] := FEvents.WorkerWaitingOnInputEvent;
   H[4] := FEvents.WorkerWaitingOnOutputEvent;
   case WaitForMultipleObjects(5, @H, False, INFINITE) of
     WAIT_OBJECT_0 + 0: FWorker.UnexpectedTerminationError;
     WAIT_OBJECT_0 + 1: FEncodeFinished := True;
-    WAIT_OBJECT_0 + 2,
+    WAIT_OBJECT_0 + 2: FProgressTimerSignaled := True;
     WAIT_OBJECT_0 + 3,
     WAIT_OBJECT_0 + 4: ;
   else
@@ -1108,6 +1140,8 @@ begin
       [FShared.InputBuffer.Count]);
 
   FEncodeStarted := False;
+  if not CancelWaitableTimer(FProgressTimer) then
+    LZMAWin32Error('CancelWaitableTimer');
 end;
 
 { TLZMA2Compressor }

Projects/Src/Compression.LZMACompressor/islzma/islzma_exe.c

@@ -10,8 +10,9 @@
   LZMA.pas revision 1.49.2.3.
 
   Intentional deviations from the original Pascal code:
-  - The WaitForMultipleObjects() calls in WakeMainAndWaitUntil and
-    BeginEncode additionally wait on ProcessData.ParentProcess.
+  - The WaitForMultipleObjects() calls in WakeMainAndWaitUntil,
+    CheckTerminateWorkerEvent, and BeginEncode additionally wait on
+    ProcessData.ParentProcess.
   Everything else *should* be 100% consistent.
 */
 
@@ -20,7 +21,7 @@
 #include "../../../../Components/Lzma2/7zTypes.h"
 #include "islzma.h"
 
-#define ISLZMA_EXE_VERSION 101
+#define ISLZMA_EXE_VERSION 102
 
 typedef BYTE Byte;
 typedef LONG Longint;
@@ -41,16 +42,14 @@ struct TLZMACompressorSharedEvents {
 	THandle32 StartEncodeEvent;
 	THandle32 EndWaitOnInputEvent;
 	THandle32 EndWaitOnOutputEvent;
-	THandle32 EndWaitOnProgressEvent;
 	THandle32 WorkerWaitingOnInputEvent;
 	THandle32 WorkerWaitingOnOutputEvent;
-	THandle32 WorkerHasProgressEvent;
 	THandle32 WorkerEncodeFinishedEvent;
 };
 
 struct TLZMACompressorSharedData {
+	volatile Int64 ProgressBytes;
 	volatile BOOL NoMoreInput;
-	volatile UInt32 ProgressKB;
 	volatile SRes EncodeResult;
 	struct TLZMACompressorRingBuffer InputBuffer;
 	struct TLZMACompressorRingBuffer OutputBuffer;
@@ -70,7 +69,6 @@ static struct TLZMACompressorProcessData ProcessData;
 static struct TLZMACompressorSharedEvents *FEvents;
 static struct TLZMACompressorSharedData *FShared;
 static volatile LONG FReadLock, FWriteLock, FProgressLock;
-static volatile DWORD FLastProgressTick;
 
 static Longint RingBufferInternalWriteOrRead(struct TLZMACompressorRingBuffer *Ring,
 	const BOOL AWrite, Longint *Offset, void *Data, Longint Size)
@@ -158,6 +156,24 @@ static HRESULT WakeMainAndWaitUntil(HANDLE AWakeEvent, HANDLE AWaitEvent)
 	}
 }
 
+static HRESULT CheckTerminateWorkerEvent(void)
+{
+	HANDLE H[2];
+
+	H[0] = THandle32ToHandle(FEvents->TerminateWorkerEvent);
+	H[1] = THandle32ToHandle(ProcessData.ParentProcess);
+	switch (WaitForMultipleObjects(2, H, FALSE, 0)) {
+		case WAIT_OBJECT_0 + 0:
+		case WAIT_OBJECT_0 + 1:
+			return E_ABORT;
+		case WAIT_TIMEOUT:
+			return S_OK;
+		default:
+			SetEvent(THandle32ToHandle(FEvents->TerminateWorkerEvent));
+			return E_FAIL;
+	}
+}
+
 static HRESULT FillBuffer(const BOOL AWrite, void *Data, size_t Size,
 	size_t *ProcessedSize)
 /* Called from worker thread (or a thread spawned by the worker thread) */
@@ -244,29 +260,20 @@ static HRESULT Write(const void *Data, size_t Size, size_t *ProcessedSize)
 static HRESULT ProgressMade(const UInt64 TotalBytesProcessed)
 /* Called from worker thread (or a thread spawned by the worker thread) */
 {
-	DWORD T;
 	HRESULT Result;
 
-	T = GetTickCount();
-	if (T - FLastProgressTick >= 100) {
-		/* Sanity check: Make sure we're the only thread inside Progress */
-		if (InterlockedExchange(&FProgressLock, 1) != 0) {
-			return E_FAIL;
-		}
-		FLastProgressTick = T;
-		/* Make sure TotalBytesProcessed isn't negative. LZMA's Types.h says
-		   "-1 for size means unknown value", though I don't see any place
-		   where LzmaEnc actually does call Progress with inSize = -1. */
-		if ((Int64)TotalBytesProcessed >= 0) {
-			FShared->ProgressKB = (UInt32)(TotalBytesProcessed >> 10);
-		}
-		Result = WakeMainAndWaitUntil(
-			THandle32ToHandle(FEvents->WorkerHasProgressEvent),
-			THandle32ToHandle(FEvents->EndWaitOnProgressEvent));
-		InterlockedExchange(&FProgressLock, 0);
-	} else {
-		Result = S_OK;
+	/* Sanity check: Make sure we're the only thread inside Progress */
+	if (InterlockedExchange(&FProgressLock, 1) != 0) {
+		return E_FAIL;
 	}
+	/* An Interlocked function is used to ensure the 64-bit value is written
+	   atomically (not with two separate 32-bit writes).
+	   TLZMACompressor will ignore negative values. LZMA SDK's 7zTypes.h says
+	   "-1 for size means unknown value", though I don't see any place
+	   where LzmaEnc actually does call Progress with inSize = -1. */
+	InterlockedExchange64(&FShared->ProgressBytes, (Int64)TotalBytesProcessed);
+	Result = CheckTerminateWorkerEvent();
+	InterlockedExchange(&FProgressLock, 0);
 
 	return Result;
 }