Sleep is evil?

[Doomulation]

I shall present my case to you and see if you can help with it a little...
There is one major issue. I made a copy extension which copies files from one place to another. But in any case, I have two threads running: the main thread which manages the GUI (pumps messages) via a loop and one thread which does the operation. The idea was that you could cancel (or click cancel), which would then show a message asking if you wanted to quit WHILE the copy progress still was continuing. Therefore I made a seperate thread for the operation. When the user clicks cancel, I also started a timer to handle the message loop, so the GUI would continue to be updated.

The issue here is that in the GUI thread,

	for(;;)
	{
		if (WaitForSingleObject(pCopyThread->m_hThread, 0) == WAIT_OBJECT_0) break;
		DoEvents(); // Pump messages
	}

This will cause a drain of cpu resources because of the endless loop. Using sleep would help that, BUT it also impairs the copy operation. It slows it down significantly. HOW then, am I supposed to make it draw less cpu cycles without hindering the copy operation? Sleep is evil.

 

[Slougi]

I would architect this a little differently, have the copy thread set some flag when it is done. Then you can have something like this:

while (copyDone == false) {
        processGuiEvents();
        thread.yield();
}

By yielding the gui thread, you wake the other thread(s). On the other hand you could just sleep for a quarter of a second or so, I'm sure that would be almost unnoticeable.

OR you could wake the gui thread only when there are actually threads to process. Not sure how to do this with win32 though

 

[Doomulation]

Yielding the thread's time slice doesn't help. Sleep(0) still causes 50% cpu load. And I need one GUI thread to keep pumping messages. Otherwise the windows will freeze.

 

[zilmar]

why not?
if (WaitForSingleObject(pCopyThread->m_hThread, 100) == WAIT_OBJECT_0) break;


why does your gui thread slow down the copying ? it should have 0 effect on it?

you could always have a look at MsgWaitForMultipleObjects, that way you are waiting for a message first before you pump.

 

[smcd]

Pause/suspend the copying (and thus the GUI updating) until the user selects yes/no to stopping the operation?

 

[Doomulation]

Pause/suspend the copying (and thus the GUI updating) until the user selects yes/no to stopping the operation?

No, not at all. If the user selects cancel and is prompted, the operation will continue smoothly. If that's what you meant.

@zilmar: Unfortunaly, it DOES hinder the other thread, and I have no clue why. There is no harm in trying WaitForMultiple... I will try.

UPDATE:
I changed the loop a little and it works much better:

	MSG msg;
	while ( GetMessage(&msg, NULL, 0, 0) )
	{
		TranslateMessage(&msg);
		DispatchMessage(&msg);
		if (WaitForSingleObject(pCopyThread->m_hThread, 0) == WAIT_OBJECT_0) break;
	}

 

[zilmar]

what was your function pump messages ?

 

[Doomulation]

Originally meant to be launched as a thread at one time, but I ditched that:

void DoEvents()
{
	DoEventsThread(NULL);
}

UINT __cdecl DoEventsThread(void* pParam)
{
	MSG msg;
	while ( PeekMessage(&msg, NULL, 0, 0, PM_REMOVE) )
	{
		TranslateMessage(&msg);
		DispatchMessage(&msg);
	}
	return 0;
}

I also found that there is a difference between copying a file over the network and to another place on another HD. The network transfer is slow using the above implemtation as well (GetMessage), but not when copying to another HD. lain:

Here is the thread that does the copying:

UINT __cdecl CopyThread(void* pParam)
{
	ProgressArguments* pArgs = (ProgressArguments*)pParam;
	CString strDir, strFileName, strExt, strCopyFrom, strCopyTo, strTemp;
	UINT64 nNumOfFiles = 0, unAdd = 0;
	bool bError = false;

	pArgs->punAdd = &unAdd;
	pArgs->pbError = &bError;

	//for (size_t i = 0; i < pArgs->pCopyClass->strFilesToCopy->size(); i++)
	//{
	//	_wsplitpath_s(pArgs->pCopyClass->strFilesToCopy->at(i), NULL, 0, strDir.GetBuffer(5000), 5000, NULL, 0, NULL, 0);
	//	strDir.ReleaseBuffer();
	//	strCopyTo = pArgs->pCopyClass->strFolderCopyTo + strDir + strFileName;
	//	strCopyTo.Replace(pArgs->pCopyClass->strFolderCopyFrom, L"");
	//	SHCreateDirectoryEx(NULL, strCopyTo, NULL);
	//}

	for (size_t i = 0; i < pArgs->pCopyClass->strFilesToCopy->size(); i++)
	{
		nPrevious = 0;
		//pArgs->pProgressDlg->nPreviousByteCountAll = 0;
		strCopyFrom = pArgs->pCopyClass->strFilesToCopy->at(i);
		
		_wsplitpath_s(strCopyFrom, NULL, 0, strDir.GetBuffer(5000), 5000, strFileName.GetBuffer(500), 500, strExt.GetBuffer(500), 500);
		strDir.ReleaseBuffer();
		strFileName.ReleaseBuffer();
		strExt.ReleaseBuffer();
		strFileName += strExt;

		strCopyTo = pArgs->pCopyClass->strFolderCopyTo + strDir;
		strCopyTo.Replace(pArgs->pCopyClass->strFolderCopyFrom, L"");
		SHCreateDirectoryEx(NULL, strCopyTo, NULL);
		strCopyTo += strFileName;

		pArgs->pProgressDlg->m_NowCopying = L"Now copying file " + strFileName + L"...";
		strTemp.Format(L"Copying file %I64u ", ++nNumOfFiles);
		if (bThreadReady)
			strTemp.AppendFormat(L"out of %I64u file(s)...", nTotalFiles);
		else
			strTemp.AppendFormat(L"...");
		pArgs->pProgressDlg->m_CopyFileOfFiles = strTemp;
	
		if (hEstimateTime) SetEvent(hEstimateTime);
		BOOL bSuccess = CopyFileEx(strCopyFrom, strCopyTo, &CopyProgressRoutine, pArgs, NULL, COPY_FILE_FAIL_IF_EXISTS);
		if (pArgs->pProgressDlg->bCancel) break;
		if (!bSuccess)
		{
			if (GetLastError() != ERROR_FILE_EXISTS)
				pArgs->pErrorDlg->m_Errors.AppendFormat(L"Unable to copy file %s (%s)...\r\n", strFileName, LastError( GetLastError() ));
			
			// Add the files size to the copied amount
			FILE* f;
			_wfopen_s(&f, pArgs->pCopyClass->strFilesToCopy->at(i), L"rb");
			if (f == NULL) continue;
			_fseeki64(f, 0, SEEK_END);
			unAdd += _ftelli64(f);
			fclose(f);

			// Recalculate the estimate time. The reason for calling the thread again is to rule out the case when the copy operations fails and it skips files. When it does, 
			// it will calculate faulty data. Therefore, we restart this thread again.
			bError = true;
			//bAbortThread = true; // This will hinder any estimation threads from setting the dwBytesPerSecond variable.
			dwBytesPerSecond = 0; // If some / any threads has already set this variable, then disable it so that faulty estimation time won't appear.
			TerminateThread(hEstTimeThread, 0); // Kill the estimation time thread
			hEstTimeThread = NULL;
			pArgs->pProgressDlg->m_EstimatedTimeAll = "Calculating...";
		}
		else;
		//{
		//	WaitForSingleObject(hEvent, INFINITE);
		//	ResetEvent(hEvent);
		//}
	}

	FILE* f;
	_wfopen_s(&f, pArgs->pCopyClass->strFilesToCopy->at( pArgs->pCopyClass->strFilesToCopy->size() - 1 ), L"rb");
	if (f != NULL)
	{
		_fseeki64(f, 0, SEEK_END);
		unAdd = _ftelli64(f);
		fclose(f);
	}
	
	if (!pArgs->pProgressDlg->bCancel)
	{
		pArgs->pProgressDlg->GetDlgItem(IDC_CANCEL)->SetWindowText(L"OK");
		pArgs->pProgressDlg->m_ProgressAll.SetPos(100);
		pArgs->pProgressDlg->m_ProgressCurrent.SetPos(100);
		pArgs->pProgressDlg->m_CopyFileOfFiles.Format(L"Copying file %I64u ", nNumOfFiles++);
		if (bThreadReady)
			pArgs->pProgressDlg->m_CopyFileOfFiles.AppendFormat(L"out of %I64u file(s)...", nTotalFiles);
		else
			pArgs->pProgressDlg->m_CopyFileOfFiles.AppendFormat(L"...");
		pArgs->pProgressDlg->m_EstimatedTimeAll = L"Estimated time reamining: 00:00:00";
		pArgs->pProgressDlg->m_EstimatedTimeCurrent = L"Estimated time reamining: 00:00:00";
		
		CString strUnit = GetSize(nTotalSize);
		pArgs->pProgressDlg->m_DataCopiedAll.Format(L"Copied %I64u %s ", nTotalSize, strUnit);
		if (bThreadReady)
			pArgs->pProgressDlg->m_DataCopiedAll.AppendFormat(L"of %I64u %s (%i%%)...", nTotalSize, strUnit, 100);
		else
			pArgs->pProgressDlg->m_DataCopiedAll += L"so far...";

		strUnit = GetSize(unAdd);
		if (unAdd != 0)
		{
			pArgs->pProgressDlg->m_DataCopiedCurrent.Format(L"Copied %I64u %s ", unAdd, strUnit);
			if (bThreadReady)
				pArgs->pProgressDlg->m_DataCopiedCurrent.AppendFormat(L"of %I64u %s (%i%%)...", unAdd, strUnit, 100);
			else
				pArgs->pProgressDlg->m_DataCopiedCurrent += L"so far...";
		}
		else
			pArgs->pProgressDlg->m_DataCopiedCurrent = L"Copied an unknown amount of data...";
	}
	return S_OK;
}

...And here is the callback routine for the copy:

DWORD CALLBACK CopyProgressRoutine(LARGE_INTEGER TotalFileSize, LARGE_INTEGER TotalBytesTransferred, LARGE_INTEGER StreamSize, LARGE_INTEGER StreamBytesTransferred, DWORD dwStreamNumber,
								   DWORD dwCallbackReason, HANDLE hSourceFile, HANDLE hDestinationFile, void* lpData)
{
	ProgressArguments* pArgs = (ProgressArguments*)lpData;
	if (pArgs->pProgressDlg->bCancel)
	{
		SetEvent(hEvent);
		return PROGRESS_CANCEL;
	}
	
	if (TotalBytesTransferred.QuadPart == 0 && (*pArgs->pnDataCopied + TotalFileSize.QuadPart) > pArgs->nFreeSpace)
	{
		pArgs->pProgressDlg->bCancel = true;
		MessageBox(NULL, L"Uh-oh! It seems that the target drive has insufficient space to continue the copy operation.\nFile copy operation has been cancelled.", L"Insufficient free space", MB_ICONERROR);
		return PROGRESS_CANCEL;
	}

	if ( (pArgs->pProgressDlg->nTimeLeftCurrent <= -3) || // Did the operation take 3 or more seconds longer than expected?
		(TotalBytesTransferred.QuadPart == TotalFileSize.QuadPart && pArgs->pProgressDlg->nTimeLeftCurrent >= 3) ) // Did the operation go 3 or more seconds faster than expected?
	{
		// Estimated time is off by at least 3 seconds, so fix it!
		*pArgs->pbError = true; // This will force the next if to recalc the estimated time.
		// Oh, and let's set the counters to calculating again.
		pArgs->pProgressDlg->m_EstimatedTimeAll = L"Calculating...";
		pArgs->pProgressDlg->m_EstimatedTimeCurrent = L"Calculating...";
		bCalcEstTimeAll = true;
		bCalcEstTimeCurrent = true;
		dwBytesPerSecond = 0;
		pArgs->pProgressDlg->nTimeLeftCurrent = 0;
	}

	if (*pArgs->pbError)
	{
		// WHOA! An error (or more) occoured and this is the first file that there does not seem to be an error with!
		SetEvent(hEstimateTime); // Since we started copying, it must be set so the estimation thread will continue.
		ResetEvent(hEstimateTimeB); // Allow estimation thread to gather number of bytes copied so far...
		bAbortThread = false;

		// I assume it's safe to say that if an error occoured and we skipped files, it couldn't have copied them, so set the new previous count to the current anount of copied byte so it won't
		// take into account the data we skipped.
		pArgs->pProgressDlg->nPreviousByteCountAll = *pArgs->pnDataCopied + *pArgs->punAdd;
		*pArgs->pnDataCopied += *pArgs->punAdd;
		*pArgs->punAdd = 0;
		*pArgs->pbError = false;

		TerminateThread(hEstTimeThread, 0); // Kill any running thread
		hEstTimeThread = AfxBeginThread(&EstimatedTimeThread, pArgs, THREAD_PRIORITY_TIME_CRITICAL)->m_hThread;
		WaitForSingleObject(hEstimateTimeB, INFINITE);
	}

	// Update progress for total files
	INT64 nTransferred = TotalBytesTransferred.QuadPart - nPrevious;
	*pArgs->pnDataCopied += nTransferred;
	UINT64 nCopiedData = *pArgs->pnDataCopied;
	UINT64 nTotalData = nTotalSize;
	nPrevious = TotalBytesTransferred.QuadPart;
	CString strUnit = GetSize(nCopiedData);
	CString strTemp;
	strTemp.Format(L"Copied %I64u %s ", nCopiedData, strUnit);
	if (bThreadReady)
	{
		ASSERT(nTotalSize != 0);
		pArgs->pProgressDlg->m_ProgressAll.SetPos( (int)( (float)*pArgs->pnDataCopied / nTotalSize * 100 ) );
		strUnit = GetSize(nTotalData);
		strTemp.AppendFormat(L"of %I64u %s (%i%%)...", nTotalData, strUnit, pArgs->pProgressDlg->m_ProgressAll.GetPos());
	}
	else
		strTemp += L"so far...";
	pArgs->pProgressDlg->m_DataCopiedAll = strTemp;

	if (dwBytesPerSecond != 0 && bCalcEstTimeAll)
	{
		// Start estimation time for all files
		UINT64 nBytes = nTotalSize - *pArgs->pnDataCopied;
		UINT64 nSeconds = nBytes / dwBytesPerSecond;
		pArgs->pProgressDlg->nTimeLeftAll = nSeconds;
		pArgs->pProgressDlg->SetTimer(2, 1000, NULL);
		bCalcEstTimeAll = false;
	}

	if (dwBytesPerSecond != 0 && bCalcEstTimeCurrent)
	{
		// Start estimation time for current file
		UINT64 nBytes = TotalFileSize.QuadPart - TotalBytesTransferred.QuadPart;
		UINT64 nSeconds = nBytes / dwBytesPerSecond;
		pArgs->pProgressDlg->nTimeLeftCurrent = nSeconds;
		pArgs->pProgressDlg->GetDlgItem(IDC_ESTIMATEDTIME_CURRENT)->ShowWindow(SW_SHOW);
		bCalcEstTimeCurrent = false;
	}

	if (TotalBytesTransferred.QuadPart == TotalFileSize.QuadPart) 
	{
		// File copy of this file is complete,
		//pArgs->pProgressDlg->GetDlgItem(IDC_ESTIMATEDTIME_CURRENT)->ShowWindow(SW_HIDE);
		bCalcEstTimeCurrent = true;
		//SetEvent(hEvent);
	}

	// Update progress for current file
	ASSERT(TotalFileSize.QuadPart != 0);
	pArgs->pProgressDlg->m_ProgressCurrent.SetPos( (int)( (float)TotalBytesTransferred.QuadPart / TotalFileSize.QuadPart * 100 ) );
	strUnit = GetSize((UINT64&)TotalBytesTransferred.QuadPart);
	CString strUnit2 = GetSize((UINT64&)TotalFileSize.QuadPart);
	pArgs->pProgressDlg->m_DataCopiedCurrent.Format(L"Copied %I64u %s of %I64u %s (%i%%)...", TotalBytesTransferred.QuadPart, strUnit, TotalFileSize.QuadPart, strUnit2,
		pArgs->pProgressDlg->m_ProgressCurrent.GetPos());

	return PROGRESS_CONTINUE;
}

I don't know if it helps anything.