Re: wininet: Remove the custom thread pool implementation and use QueueUserWorkItem instead.

[Jacek Caban]

Hi Rob,

Robert Shearman wrote:
> ---
>  dlls/wininet/internet.c |  184 
> +++++------------------------------------------
>  dlls/wininet/internet.h |    3 -
>  2 files changed, 19 insertions(+), 168 deletions(-)
>
> This also fixes a bug in the old code where starting a new 
> asynchronous operation with more than 10 asynchronous operations in 
> progress would fail.
> ...
> -    LPWORKREQUEST lpRequest = NULL;
> +    LPWORKREQUEST lpRequest = lpvParam;
> +    WORKREQUEST workRequest;
>
> TRACE("\n");
>
> -    EnterCriticalSection(&csQueue);
> +    memcpy(&workRequest, lpRequest, sizeof(WORKREQUEST));
> +    HeapFree(GetProcessHeap(), 0, lpRequest);
>
> -    if (lpHeadWorkQueue)
> -    {
> -        lpRequest = lpHeadWorkQueue;
> -        lpHeadWorkQueue = lpHeadWorkQueue->prev;
> -	if (lpRequest == lpWorkQueueTail)
> -            lpWorkQueueTail = lpHeadWorkQueue;
> -    }
> -
> -    LeaveCriticalSection(&csQueue);
> -
> -    if (lpRequest)
> -    {
> -        memcpy(lpWorkRequest, lpRequest, sizeof(WORKREQUEST));
> -        HeapFree(GetProcessHeap(), 0, lpRequest);
> -	bSuccess = TRUE;
> -	InterlockedDecrement(&dwNumJobs);
> -    }
> +    workRequest.asyncproc(&workRequest);
>
> -    return bSuccess;
> +    WININET_Release( workRequest.hdr );
> +    return TRUE;
>  }

Why do you copy lpRequest to workRequest? You could use lpRequest
in asyncproc and WININET_Release and call HeapFree before return.
Then workRequest may be removed.

Thanks,
    Jacek

wininet: Remove the custom thread pool implementation and use QueueUserWorkItem instead.

[Robert Shearman]

Hi Jacek,

Thanks for looking through the patch.

Jacek Caban wrote:
> Why do you copy lpRequest to workRequest? You could use lpRequest
> in asyncproc and WININET_Release and call HeapFree before return.
> Then workRequest may be removed.

Two reasons:
1. This was how the code did it before - I copied it from 
INTERNET_ExecuteWork/INTERNET_GetWorkRequest.
2. The stack is already allocated, so using it instead of the heap makes 
sense since it allows the block in the heap to be used again immediately.

Yes, it's two more lines of code, but I can't see a convincing reason to 
change it from how it was before. Can you?

-- 
Rob Shearman

wininet: Remove the custom thread pool implementation and use QueueUserWorkItem instead.

[Jacek Caban]

Robert Shearman wrote:
> Hi Jacek,
>
> Thanks for looking through the patch.
>
> Jacek Caban wrote:
> > Why do you copy lpRequest to workRequest? You could use lpRequest
> > in asyncproc and WININET_Release and call HeapFree before return.
> > Then workRequest may be removed.
>
> Two reasons:
> 1. This was how the code did it before - I copied it from 
> INTERNET_ExecuteWork/INTERNET_GetWorkRequest.
> 2. The stack is already allocated, so using it instead of the heap 
> makes sense since it allows the block in the heap to be used again 
> immediately.
>
> Yes, it's two more lines of code, but I can't see a convincing reason 
> to change it from how it was before. Can you?
> Both reasons don't convince. The previous code copied it indirectly in a 
> called
> function, but, nevertheless, those two lines of code are not worth 
> discussion.
> Sorry for the noise. I was planning to change the requests code a bit. You
> already beat me to this patch. I also want to get rid of union in WORKREQ
> and this change requires passing pointers everywhere around request 
> functions.

Thanks,
    Jacek