|
From: Aleksander S. <A....@os...> - 2004-03-15 10:09:17
Attachments:
vg_libpthread.c.patch
|
Here is a patch that implements semaphore remapping in constant time. Function sem_destroy really works now. Applications that create and destroy a lot of semaphores will work (you may still need to increase VG_N_SEMAPHORES if you use a lot of semaphores at a time). Performance is better because of: - simple pointer look-ups in se_new, se_remap, se_unmap - no linear search - locking is no longer needed in se_remap Storing pointers to V's internal data in user's sem_t seems to be an advantage, not disadvantage, because: - now Valgrind can detect overwrites of user's sem_t (additional error checking - SEM_CHECK_MAGIC) - previously it would go undetected - if invalid pointer is passed to one of sem_xxx functions, Valgrind will report invalid memory access in se_remap or se_new - previously se_remap would silently allocate new semaphore Patch is attached. Could someone with write access commit it into CVS ? Olek S. |
|
From: Jeremy F. <je...@go...> - 2004-03-15 16:22:42
|
On Mon, 2004-03-15 at 02:09, Aleksander Salwa wrote: > Here is a patch that implements semaphore remapping in constant time. > Function sem_destroy really works now. Applications that create and > destroy a lot of semaphores will work (you may still need to increase > VG_N_SEMAPHORES if you use a lot of semaphores at a time). > > Performance is better because of: > - simple pointer look-ups in se_new, se_remap, se_unmap - no linear > search > - locking is no longer needed in se_remap > > Storing pointers to V's internal data in user's sem_t seems to be an > advantage, not disadvantage, because: > - now Valgrind can detect overwrites of user's sem_t (additional error > checking - SEM_CHECK_MAGIC) - previously it would go undetected > - if invalid pointer is passed to one of sem_xxx functions, Valgrind > will report invalid memory access in se_remap or se_new - previously > se_remap would silently allocate new semaphore > > Patch is attached. Could someone with write access commit it into CVS ? It looks pretty good, but I have a few comments: Why still keep the big array? Can't this be made dynamic so that there's no need for VG_N_SEMAPHORES? Also, barf() seems to be mostly used for reporting vg_libpthread internal errors, where as the places you use it seem to be all client bugs; maybe just using VG_INTERNAL_PRINTF_BACKTRACE. And finally, could you write a regression test case and add it to none/tests? Something which would have failed before, but now works, and exercises as much of the semaphore machinery as possible. Thanks, J |
|
From: Aleksander S. <A....@os...> - 2004-03-16 09:59:46
Attachments:
vg_libpthread.c.patch
|
On Mon, 2004-03-15 at 17:22, Jeremy Fitzhardinge wrote: > It looks pretty good, but I have a few comments: > > Why still keep the big array? Can't this be made dynamic so that > there's no need for VG_N_SEMAPHORES? Maybe I will implement this in the future. But first I have to perform benchmarks to check performance penalty caused by additional malloc in every sem_init. It is really important in my use cases. Maybe I will allocate memory for vg_sem_t structures in big chunks, I don't know yet. If I will find the time, I will write an allocation scheme that eliminates VG_N_SEMAPHORES. > Also, barf() seems to be mostly used for reporting vg_libpthread > internal errors, where as the places you use it seem to be all client > bugs; maybe just using VG_INTERNAL_PRINTF_BACKTRACE. I've decided to change that barf() to pthread_error(). It seems most reasonable in these cases. Also added setting of errno to EINVAL - according to POSIX. > And finally, could you write a regression test case and add it to > none/tests? Something which would have failed before, but now works, > and exercises as much of the semaphore machinery as possible. I'm very busy, so it may take a week or two before I find the time for it. But I hope I will do it. New patch (eliminated barf()) attached. I hope you can commit it now, as it only makes things better - elimination of VG_N_SEMAPHORES may be done later. Olek S. |