May 2011 - Wine-Devel - list.winehq.org

Re: [2/2] comctl32/mru: Ignore size field when creating with -W calls

by Nikolay Sivov

On 5/2/2011 14:45, Alexandre Julliard wrote: > Nikolay Sivov<nsivov(a)codeweavers.com> writes: > >> @@ -210,11 +210,11 @@ typedef INT (CALLBACK *MRUStringCmpFnA)(LPCSTR lhs, LPCSTR rhs); >> typedef INT (CALLBACK *MRUStringCmpFnW)(LPCWSTR lhs, LPCWSTR rhs); >> typedef INT (CALLBACK *MRUBinaryCmpFn)(LPCVOID lhs, LPCVOID rhs, DWORD length); >> >> -typedef struct tagCREATEMRULISTA >> +typedef struct tagMRUINFOA >> { >> DWORD cbSize; >> - DWORD nMaxItems; >> - DWORD dwFlags; >> + UINT uMax; >> + UINT fFlags; >> HKEY hKey; >> LPSTR lpszSubKey; >> union > Please do the renamings as a separate patch. Done.

May 2, 2011

Re: D3D performance debugging report

by Henri Verbeet

On 2 May 2011 13:20, Emanuele Oriani <emaentra(a)ngi.it> wrote: > Hi, let me agree with you... probably there might be one or two fixes > somewhere which will make performance go better, but apparently the idea is > that now, if "early optimization is the root of all evils" was true a while > a go, I guess we're at a place where all the small optimizations should take > place. There's a difference between "early" and "premature", but I guess it's subtle. I don't agree with the concept of "Anything goes as long as it improves performance for someone, somewhere." > For example, the main issue with SC2 is that if you don't set the affinity > of all threads to one core, the game will go slower by 40%; for this reason > I think this game suffers of other issues related to other wine components > (than D3D) and/or Linux scheduler. > The main issue with SC2 is that setting different shaders level will heavily > impact the game; and I'm running on a 470 GTX: this shouldn't be the case. > Should we probably looking into the way we generate shaders (both GLSlang > and ARB ones)? > Another point, what about how we handle offscreen buffers? All FBO etc etc? > Most of it really does come down to careful debugging / benchmarking, I'm afraid. There's plenty of speculation, but at some point someone just has to do the actual work. > If I remember correctly someone was working on a worker thread for D3D. Did > we abandon tis project? Not as such, but to my knowledge nobody is currently actively working on that. This is also one of those things that's a lot harder than it looks.

May 2, 2011

Re: [PATCH 2/2] wined3d: Turn off message filtering temporarily in the Reset() method to allow certain messages (e.g. WM_ACTIVATEAPP) through.

by Henri Verbeet

On 2 May 2011 13:18, John Edmonds <pocketcookies2(a)gmail.com> wrote: > Toki Tori and RUSH both depend on receing WM_ACTIVATEAPP before they will start. These messages are currently getting filtered out. > This resolves bug #25640. It doesn't look right to me to filter messages during IWineD3DDevice_ReleaseFocusWindow() but not IWineD3DDevice_AcquireFocusWindow(). Maybe it just shouldn't filter at all.

May 2, 2011

Re: [PATCH 1/2] d3d9/tests: Added a test for resetting an invisible, inactive window to fullscreen mode.

by Henri Verbeet

On 2 May 2011 13:18, John Edmonds <pocketcookies2(a)gmail.com> wrote: > + /*Pull all currently available messages. We don't care about messages generated after this loop exits.*/ > + while(PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) > + { > + TranslateMessage(&msg); > + DispatchMessage(&msg); > + } You should probably use flush_events() for that.

May 2, 2011

Re: [PATCH 2/2] wined3d: Turn off message filtering temporarily in the Reset() method to allow certain messages (e.g. WM_ACT

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=10678 Your paranoid android. === WXPPROSP3 (32 bit device) === device: unhandled exception c0000005 at 0040320F === W2K3R2SESP2 (32 bit device) === device: unhandled exception c0000005 at 0040320F === WVISTAADM (32 bit device) === device: unhandled exception c0000005 at 0040320F === W2K8SE (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PRO (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PROX64 (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PROX64 (64 bit device) === Timeout

May 2, 2011

Re: [PATCH 1/2] d3d9/tests: Added a test for resetting an invisible, inactive window to fullscreen mode.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=10677 Your paranoid android. === WXPPROSP3 (32 bit device) === device: unhandled exception c0000005 at 0040320F === W2K3R2SESP2 (32 bit device) === device: unhandled exception c0000005 at 0040320F === WVISTAADM (32 bit device) === device: unhandled exception c0000005 at 0040320F === W2K8SE (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PRO (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PROX64 (32 bit device) === device: unhandled exception c0000005 at 0040320F === W7PROX64 (64 bit device) === Timeout

May 2, 2011

Re: D3D performance debugging report

by Emanuele Oriani

Hi, let me agree with you... probably there might be one or two fixes somewhere which will make performance go better, but apparently the idea is that now, if "early optimization is the root of all evils" was true a while a go, I guess we're at a place where all the small optimizations should take place. To confirm, SC2 performance difference without the D3D lock and the x11 spinlock, compared to current wine (1.3.19) is basically not noticeable. On top of this, be aware that apparently the D3D lock IS recursive (when using the non recursive spinlock a CPU was going 100% and application was stuck). For example, the main issue with SC2 is that if you don't set the affinity of all threads to one core, the game will go slower by 40%; for this reason I think this game suffers of other issues related to other wine components (than D3D) and/or Linux scheduler. The main issue with SC2 is that setting different shaders level will heavily impact the game; and I'm running on a 470 GTX: this shouldn't be the case. Should we probably looking into the way we generate shaders (both GLSlang and ARB ones)? Another point, what about how we handle offscreen buffers? All FBO etc etc? If I remember correctly someone was working on a worker thread for D3D. Did we abandon tis project? Given OpenGL doesn't (didn't?) support calls from multiple threads, should we be proceeding through this route? Cheers, On 01/05/11 15:10, Stefan Dösinger wrote: > On Sunday 01 May 2011 14:34:53 Emanuele Oriani wrote: >> Indeed, I've written a spinlock with GCC extension and replaced the >> EnterCriticalSection in the x11 drv file. >> Apart that the lock has got to be recursive, so I implemented a quick >> (but incorrect) recursive spinlock for the purpose of running SC2 and >> difference was barely negligible. > How much was the difference? > >> The biggest issue imho is that in this case we have to call a >> function... > I don't think so. I did some tests for the call overhead, and it is fairly > small. Specifically I tried to export the wined3d lock from wined3d and call > EnterCriticalSection / LeaveCritSection directly from d3d9. The difference > wasn't even measurable with my hyper-sensitive self-written test apps. > >> I can try a spinlock for the BKL-like which is wined3d lock. I hope this >> hasn't got to be recursive, right? >> I'm asking this because in case of a recursive lock I'm performing an >> extra syscall: > The wined3d lock doesn't have to be recursive I think. But note that getting > those changes committed into Wine are next to zero. It's more likely to get an > optimization of EnterCriticalSection / LeaveCriticalSection itself into wine. > >> Please keep in mind this is a test code, but apparently it's working. >> Again, performance in case of SC2 isn't that much... but probably should >> test better/with other games? > No, as I explained in my mail the individual optimizations don't magically fix > all the performance woes we have. We'll probably have to collect a dozen or > more such little fixes to start seeing movement. >

May 2, 2011

Re: wininet: Use test.winehq.org for post tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=10676 Your paranoid android. === WVISTAADM (32 bit http) === Failure running script in VM: The specified guest user must be logged in interactively to perform this operation

May 2, 2011

Re: [PATCH 1/5] mmdevapi: Add InitializeDriver() method to drivers

by Alexandre Julliard

Paul Chitescu <paulc(a)voip.null.ro> writes: > Wine is already plagued by deadlocks in the MM drivers initialization that > interfere with the loader's lock. Explicit initialization after LoadLibrary > returns should help getting rid of these. Considering that the MM drivers don't implement DLL_PROCESS_ATTACH at all at the moment, I don't think avoiding it is going to fix anything... -- Alexandre Julliard julliard(a)winehq.org

May 2, 2011

Re: [PATCH 1/5] mmdevapi: Add InitializeDriver() method to drivers

by Paul Chitescu

On Monday 02 May 2011 01:32:59 pm Alexandre Julliard wrote: > Andrew Eikum <aeikum(a)codeweavers.com> writes: > > @@ -72,11 +73,19 @@ static BOOL load_driver(const WCHAR *name) > > } > > > > #define LDFC(n) do { drvs.p##n = (void*)GetProcAddress(drvs.module, > > #n);\ - if(!drvs.p##n) return FALSE; } while(0); > > + if(!drvs.p##n) { FreeLibrary(drvs.module); return FALSE; } } > > while(0); + LDFC(InitializeDriver); > > LDFC(GetEndpointIDs); > > LDFC(GetAudioEndpoint); > > #undef LDFC > > > > + hr = drvs.pInitializeDriver(); > > + if(FAILED(hr)){ > > + TRACE("Driver initialization failed: %08x\n", hr); > > + FreeLibrary(drvs.module); > > + return hr; > > + } > > If you are going to call it on load, you could just as well do the init > in the driver on DLL_PROCESS_ATTACH and avoid the extra method. Wine is already plagued by deadlocks in the MM drivers initialization that interfere with the loader's lock. Explicit initialization after LoadLibrary returns should help getting rid of these.

May 2, 2011

Re: [2/2] comctl32/mru: Ignore size field when creating with -W calls

by Alexandre Julliard

Nikolay Sivov <nsivov(a)codeweavers.com> writes: > @@ -210,11 +210,11 @@ typedef INT (CALLBACK *MRUStringCmpFnA)(LPCSTR lhs, LPCSTR rhs); > typedef INT (CALLBACK *MRUStringCmpFnW)(LPCWSTR lhs, LPCWSTR rhs); > typedef INT (CALLBACK *MRUBinaryCmpFn)(LPCVOID lhs, LPCVOID rhs, DWORD length); > > -typedef struct tagCREATEMRULISTA > +typedef struct tagMRUINFOA > { > DWORD cbSize; > - DWORD nMaxItems; > - DWORD dwFlags; > + UINT uMax; > + UINT fFlags; > HKEY hKey; > LPSTR lpszSubKey; > union Please do the renamings as a separate patch. -- Alexandre Julliard julliard(a)winehq.org

May 2, 2011

Re: [1/5] cmd: WCMD_delete: simplify logic, avoid assigning variable to itself

by Henri Verbeet

On 2 May 2011 02:23, Dan Kegel <dank(a)kegel.com> wrote: > - if (attribute && !negate) { > - stillOK = stillOK; > - } else if (!attribute && negate) { > - stillOK = stillOK; > - } else { > + if (attribute == negate) > stillOK = FALSE; > - } If you just write that as "if (!attribute == !negate)" you avoid both the assumption that TRUE is defined to 1 and the ugly "!= 0"'s. Or you could build actual attribute masks first and compare those instead, which may even be readable.

May 2, 2011

Re: [PATCH 1/5] mmdevapi: Add InitializeDriver() method to drivers

by Alexandre Julliard

Andrew Eikum <aeikum(a)codeweavers.com> writes: > @@ -72,11 +73,19 @@ static BOOL load_driver(const WCHAR *name) > } > > #define LDFC(n) do { drvs.p##n = (void*)GetProcAddress(drvs.module, #n);\ > - if(!drvs.p##n) return FALSE; } while(0); > + if(!drvs.p##n) { FreeLibrary(drvs.module); return FALSE; } } while(0); > + LDFC(InitializeDriver); > LDFC(GetEndpointIDs); > LDFC(GetAudioEndpoint); > #undef LDFC > > + hr = drvs.pInitializeDriver(); > + if(FAILED(hr)){ > + TRACE("Driver initialization failed: %08x\n", hr); > + FreeLibrary(drvs.module); > + return hr; > + } If you are going to call it on load, you could just as well do the init in the driver on DLL_PROCESS_ATTACH and avoid the extra method. -- Alexandre Julliard julliard(a)winehq.org

May 2, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 21:55 +0400, Nikolay Sivov wrote: > On 5/1/2011 21:49, Alexey Fisher wrote: > > On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: > >> On 5/1/2011 13:07, Alexey Fisher wrote: > >>> There is a new patch in the attachment. > >>> BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > >>> { > >>> - TRACE("(%p %s)\n", lppDest, lpSrc); > >>> + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > >>> + > >>> + if (*lppDest) { > >>> + ERR("lppDest should be NULL!"); > >>> + return FALSE; > >>> + } > >> It's an internal call, so it's better to require a caller to pass valid > >> parameters. That's why ERR is too much here, cause you completely > >> control passed parameters in a first place. > > The problem is, this function return FALSE if some thing going wrong, > > but caller never check it. So if we pass fresh not NULL, not Alloc'd > > pointer. We have some garbage in pointers target before the call. After > > ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers > > target, also old garbage. The App get garbage instead of the string and > > will crash in some conditions. This all make it hard to find the cause > > of the crash. > I'll try to explain again. > > Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside > comctl32. My request to you was to provide a simple test application that uses some comctl32 controls > presumably and indirectly makes use of this call (and fails for some reason you'll trying to find). > > >> Please open a bug with a test C application so we can look at initial > >> problem. > > Das Not ReAllocing not Alloc'd memory is bug? or it is future? > It depends. > > Here is part of man realloc: > > > >> realloc() changes the size of the memory block pointed to by ptr to > >> size bytes. The contents will be unchanged to the minimum of the old > >> and new sizes; newly allocated memory will be uninitialized. If ptr is > >> NULL, then the call is equivalent to malloc(size), for all values of > >> size; if size is equal to zero, and ptr is not NULL, then the call is > >> equivalent to free(ptr). Unless ptr is NULL, it must have been > >> returned by an earlier call to malloc(), calloc() or realloc(). If the > >> area pointed to was moved, a free(ptr) is done. > > This is irrelevant here. I need to apologize. Wine a big mystery for me. And it seems like the string corruption i talked about, i introduced by my self. And i see you send the patch to test the strings: comctl32/tests: Cleanup MRU lists tests, add more At least i got the target and found how to workaround my bug http://bugs.winehq.org/show_bug.cgi?id=26923 :D -- Regards, Alexey

May 2, 2011

Re: comctl32: realloc or free and alloc

by Nikolay Sivov

On 5/1/2011 21:49, Alexey Fisher wrote: > On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: >> On 5/1/2011 13:07, Alexey Fisher wrote: >>> There is a new patch in the attachment. >>> BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) >>> { >>> - TRACE("(%p %s)\n", lppDest, lpSrc); >>> + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); >>> + >>> + if (*lppDest) { >>> + ERR("lppDest should be NULL!"); >>> + return FALSE; >>> + } >> It's an internal call, so it's better to require a caller to pass valid >> parameters. That's why ERR is too much here, cause you completely >> control passed parameters in a first place. > The problem is, this function return FALSE if some thing going wrong, > but caller never check it. So if we pass fresh not NULL, not Alloc'd > pointer. We have some garbage in pointers target before the call. After > ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers > target, also old garbage. The App get garbage instead of the string and > will crash in some conditions. This all make it hard to find the cause > of the crash. I'll try to explain again. Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside comctl32. My request to you was to provide a simple test application that uses some comctl32 controls presumably and indirectly makes use of this call (and fails for some reason you'll trying to find). >> Please open a bug with a test C application so we can look at initial >> problem. > Das Not ReAllocing not Alloc'd memory is bug? or it is future? It depends. > Here is part of man realloc: > >> realloc() changes the size of the memory block pointed to by ptr to >> size bytes. The contents will be unchanged to the minimum of the old >> and new sizes; newly allocated memory will be uninitialized. If ptr is >> NULL, then the call is equivalent to malloc(size), for all values of >> size; if size is equal to zero, and ptr is not NULL, then the call is >> equivalent to free(ptr). Unless ptr is NULL, it must have been >> returned by an earlier call to malloc(), calloc() or realloc(). If the >> area pointed to was moved, a free(ptr) is done. > This is irrelevant here.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: > On 5/1/2011 13:07, Alexey Fisher wrote: > > > > There is a new patch in the attachment. > > > BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > > { > > - TRACE("(%p %s)\n", lppDest, lpSrc); > > + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > > + > > + if (*lppDest) { > > + ERR("lppDest should be NULL!"); > > + return FALSE; > > + } > It's an internal call, so it's better to require a caller to pass valid > parameters. That's why ERR is too much here, cause you completely > control passed parameters in a first place. The problem is, this function return FALSE if some thing going wrong, but caller never check it. So if we pass fresh not NULL, not Alloc'd pointer. We have some garbage in pointers target before the call. After ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers target, also old garbage. The App get garbage instead of the string and will crash in some conditions. This all make it hard to find the cause of the crash. > Please open a bug with a test C application so we can look at initial > problem. Das Not ReAllocing not Alloc'd memory is bug? or it is future? Here is part of man realloc: > realloc() changes the size of the memory block pointed to by ptr to > size bytes. The contents will be unchanged to the minimum of the old > and new sizes; newly allocated memory will be uninitialized. If ptr is > NULL, then the call is equivalent to malloc(size), for all values of > size; if size is equal to zero, and ptr is not NULL, then the call is > equivalent to free(ptr). Unless ptr is NULL, it must have been > returned by an earlier call to malloc(), calloc() or realloc(). If the > area pointed to was moved, a free(ptr) is done. -- Regards, Alexey

May 1, 2011

Re: comctl32: realloc or free and alloc

by Vitaliy Margolen

On 05/01/2011 10:29 AM, Alexey Fisher wrote: > Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated > the memory by Alloc. At least for glibc. First of all we talking about Wine's functions HeapAlloc, HeapReAlloc, and HeapFree. The HeapReAlloc can and does re-allocate memory initially allocated by HeapAlloc. However it can not allocate memory first time around - calling HeapReAlloc with null src pointer is an error. What Dan was talking about - HeapReAlloc will attempt to reuse the same memory if new size is same or smaller then current size. This can significantly improve performance. Vitaliy.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 08:05 -0700, Dan Kegel wrote: > On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher > <bug-track(a)fisher-privat.net> wrote: > >> > The code looks like it assumes that pointer is always > >> > managed by Alloc/ReAlloc/Free. In what context is > >> > the destination a fresh, non-Alloc'd pointer? > >> > Perhaps that's where the bug lies. > >> > >> We do not need to save anything what was in the target. Even the old > >> code ReAloocate it and then rewrite. In most contexts the pointer is a > >> fresh one. If it is not, it should be freed before this func. > > I don't see why we should need to change the contract > so much. Re-using existing blocks of memory isn't > the worst idea in the world. Probably not, but: Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated the memory by Alloc. At least for glibc. > > There is a new patch in the attachment. > > It seems there's still some mystery in the code, > and that you don't fully understand it, so your > patch probably isn't right. you right. I'll continue to digg and send new patch ASAP. -- Regards, Alexey

May 1, 2011

Re: comctl32: realloc or free and alloc

by Nikolay Sivov

On 5/1/2011 13:07, Alexey Fisher wrote: > > There is a new patch in the attachment. > BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > { > - TRACE("(%p %s)\n", lppDest, lpSrc); > + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > + > + if (*lppDest) { > + ERR("lppDest should be NULL!"); > + return FALSE; > + } It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place. Please open a bug with a test C application so we can look at initial problem.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Dan Kegel

On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher <bug-track(a)fisher-privat.net> wrote: >> > The code looks like it assumes that pointer is always >> > managed by Alloc/ReAlloc/Free. In what context is >> > the destination a fresh, non-Alloc'd pointer? >> > Perhaps that's where the bug lies. >> >> We do not need to save anything what was in the target. Even the old >> code ReAloocate it and then rewrite. In most contexts the pointer is a >> fresh one. If it is not, it should be freed before this func. I don't see why we should need to change the contract so much. Re-using existing blocks of memory isn't the worst idea in the world. > There is a new patch in the attachment. It seems there's still some mystery in the code, and that you don't fully understand it, so your patch probably isn't right. - Dan

May 1, 2011

Re: D3D performance debugging report

by Stefan Dösinger

On Sunday 01 May 2011 14:34:53 Emanuele Oriani wrote: > Indeed, I've written a spinlock with GCC extension and replaced the > EnterCriticalSection in the x11 drv file. > Apart that the lock has got to be recursive, so I implemented a quick > (but incorrect) recursive spinlock for the purpose of running SC2 and > difference was barely negligible. How much was the difference? > The biggest issue imho is that in this case we have to call a > function... I don't think so. I did some tests for the call overhead, and it is fairly small. Specifically I tried to export the wined3d lock from wined3d and call EnterCriticalSection / LeaveCritSection directly from d3d9. The difference wasn't even measurable with my hyper-sensitive self-written test apps. > I can try a spinlock for the BKL-like which is wined3d lock. I hope this > hasn't got to be recursive, right? > I'm asking this because in case of a recursive lock I'm performing an > extra syscall: The wined3d lock doesn't have to be recursive I think. But note that getting those changes committed into Wine are next to zero. It's more likely to get an optimization of EnterCriticalSection / LeaveCriticalSection itself into wine. > Please keep in mind this is a test code, but apparently it's working. > Again, performance in case of SC2 isn't that much... but probably should > test better/with other games? No, as I explained in my mail the individual optimizations don't magically fix all the performance woes we have. We'll probably have to collect a dozen or more such little fixes to start seeing movement.

May 1, 2011

Re: D3D performance debugging report

by Emanuele Oriani

Indeed, I've written a spinlock with GCC extension and replaced the EnterCriticalSection in the x11 drv file. Apart that the lock has got to be recursive, so I implemented a quick (but incorrect) recursive spinlock for the purpose of running SC2 and difference was barely negligible. The biggest issue imho is that in this case we have to call a function... it would be great to inline all that code, but again, probably the best thing is to limit the number of calls. I can try a spinlock for the BKL-like which is wined3d lock. I hope this hasn't got to be recursive, right? I'm asking this because in case of a recursive lock I'm performing an extra syscall: static volatile pid_t x11_lock = 0; static volatile int x11_lock_cnt = 0; /*********************************************************************** * wine_tsx11_lock (X11DRV.@) */ void CDECL wine_tsx11_lock(void) { pid_t th_id = syscall(SYS_gettid); // This might be expensive! // I don't like recursive locks for this reason! while (th_id != __sync_val_compare_and_swap(&x11_lock, 0, th_id)); ++x11_lock_cnt; asm volatile("lfence" ::: "memory"); } /*********************************************************************** * wine_tsx11_unlock (X11DRV.@) */ void CDECL wine_tsx11_unlock(void) { if(!--x11_lock_cnt) x11_lock=0; asm volatile("sfence" ::: "memory"); } Please keep in mind this is a test code, but apparently it's working. Again, performance in case of SC2 isn't that much... but probably should test better/with other games? Let me know, Cheers, On 01/05/11 09:33, Stefan Dösinger wrote: > On Saturday 30 April 2011 18:26:04 Emanuele Oriani wrote: >> Hi Stefan, >> >> What do you think about using inline spinlocks (in asm code maybe) to >> implement locks? >> Clearly an optimized spinlock would mean different code for different >> compilers/architectures, but shouldn't it be the best solution? > I am usually pessimistic about hand-written assembler optimizations. You can > give it a try, but compilers are pretty clever these days. > > I think trying to optimize the lock calls is a more promising way. We can't > simply drop the ENTER_GL/LEAVE_GL calls, as you found out in SC2. We may be > able to reduce the number of those calls by moving blocks of opengl calls > closer together. > > There's also the wined3d lock, which is somewhat like the big kernel lock. > There's room for improvement there as well, if we soften the "you must call > wined3d under lock" rule. However the wined3d lock is the smaller problem > compared to the X11 lock.

May 1, 2011

re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 07:43 +0200, Alexey Fisher wrote: > Am Samstag, den 30.04.2011, 14:41 +0000 schrieb Dan Kegel: > > Alexey wrote: > > > > > i currently digg in comctl32 to find why my app fails. I found that > > > string conversation AtoW in some places silently fails. The problem is > > > that the destination for string is just a fresh pointer (not NULL). > > > Str_SetPtrAtoW check if it is NULL pointer and if not it trys to > > > ReAlloc. There was no Alloc before so ReAlloc returns NULL > > > > The code looks like it assumes that pointer is always > > managed by Alloc/ReAlloc/Free. In what context is > > the destination a fresh, non-Alloc'd pointer? > > Perhaps that's where the bug lies. > > We do not need to save anything what was in the target. Even the old > code ReAloocate it and then rewrite. In most contexts the pointer is a > fresh one. If it is not, it should be freed before this func. > > > Your proposed solution would pass a non-Alloc'd > > pointer to Free, which doesn't seem good. > > So probably best solution is to forbid using not a NULL pointer in this > func. The programmer should decide where to Free it before. > There is a new patch in the attachment. -- Regards, Alexey

May 1, 2011

Re: [PATCH 4/5] winecoreaudio.drv: Implement IAudioSessionControl::GetState

by Ken Thomases

Hi, On Apr 29, 2011, at 3:50 PM, Andrew Eikum wrote: > + LIST_FOR_EACH_ENTRY(client, &This->session->clients, ACImpl, entry){ > + OSSpinLockLock(&client->lock); > + if(client->playing == StatePlaying){ > + *state = AudioSessionStateActive; I think the session should be considered active if a client is in StateInTransition, too. Regards, Ken

May 1, 2011

Re: D3D performance debugging report

by Stefan Dösinger

On Saturday 30 April 2011 18:26:04 Emanuele Oriani wrote: > Hi Stefan, > > What do you think about using inline spinlocks (in asm code maybe) to > implement locks? > Clearly an optimized spinlock would mean different code for different > compilers/architectures, but shouldn't it be the best solution? I am usually pessimistic about hand-written assembler optimizations. You can give it a try, but compilers are pretty clever these days. I think trying to optimize the lock calls is a more promising way. We can't simply drop the ENTER_GL/LEAVE_GL calls, as you found out in SC2. We may be able to reduce the number of those calls by moving blocks of opengl calls closer together. There's also the wined3d lock, which is somewhat like the big kernel lock. There's room for improvement there as well, if we soften the "you must call wined3d under lock" rule. However the wined3d lock is the smaller problem compared to the X11 lock.

May 1, 2011

Re: ddraw: Add a few tests for Setcooperativelevel

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=10649 Your paranoid android. === WNT4WSSP6 (32 bit ddrawmodes) === ddrawmodes.c:800: Test failed: SetCooperativelevel(DDSCL_SETFOCUSWINDOW) returned: 80070057 ddrawmodes.c:803: Test failed: SetCooperativelevel(DDSCL_CREATEDEVICEWINDOW | DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN) returned: 80070057 ddrawmodes.c:828: Test failed: SetCooperativelevel(DDSCL_SETFOCUSWINDOW) returned: 80070057 === W7PROX64 (64 bit ddrawmodes) === Timeout

May 1, 2011

re: comctl32: realloc or free and alloc

by Alexey Fisher

Am Samstag, den 30.04.2011, 14:41 +0000 schrieb Dan Kegel: > Alexey wrote: > > > i currently digg in comctl32 to find why my app fails. I found that > > string conversation AtoW in some places silently fails. The problem is > > that the destination for string is just a fresh pointer (not NULL). > > Str_SetPtrAtoW check if it is NULL pointer and if not it trys to > > ReAlloc. There was no Alloc before so ReAlloc returns NULL > > The code looks like it assumes that pointer is always > managed by Alloc/ReAlloc/Free. In what context is > the destination a fresh, non-Alloc'd pointer? > Perhaps that's where the bug lies. We do not need to save anything what was in the target. Even the old code ReAloocate it and then rewrite. In most contexts the pointer is a fresh one. If it is not, it should be freed before this func. > Your proposed solution would pass a non-Alloc'd > pointer to Free, which doesn't seem good. So probably best solution is to forbid using not a NULL pointer in this func. The programmer should decide where to Free it before.

May 1, 2011