[Committed] POSIX Compliant "TaskSet" locking
Forum rules
*******************************
Please be aware we are now using GitHub for issue tracking and feature requests.
- This section of the forum is now closed to new topics.
*******************************
*******************************
Please be aware we are now using GitHub for issue tracking and feature requests.
- This section of the forum is now closed to new topics.
*******************************
[Committed] POSIX Compliant "TaskSet" locking
Perhaps a year ago, there was a discussion on IRC about the locking in decomb.c/deinterlace.c and it relying on undefined/implementation specific behavior of POSIX mutexes. That is, the ability for one thread to unlock a mutex owned by another thread. This particular behavior is explicitly disallowed by the FreeBSD pthreads implementation (it will terminate the program). To avoid this, I used POSIX condition variables to control the ganged computation in HandBrake. The original set of patches, which are included in the FreeBSD ports collection version of HandBrake, had too much code duplication for my tastes, so I never submitted them for inclusion in HandBrake. I've now re-factored the code into something I hope will be acceptable. My patches can be found here:
http://pastebin.ca/1817937
The code adds the concept of a taskset: a group of cooperating threads, working on the same task. The taskset API encapsulates all of the tedium of managing the threads and synchronization primitives. A consumer of taskset, need only create, cycle for each unit or work, and destroy the taskset in order to complete the task. The threads of the task are provided with two simple functions to wait for work and to signal work completion. The use of the API allows for a ~200 line reduction in decomb.c alone.
When this was last discussed, concerns were raised about performance. Since the original implementation does not run on FreeBSD, I do not have before and after comparisons. I have access to Linux VMs on a Xen system, and I can run before and after tests there, but I fear that the introduction of virtualization could make the results unreliable. My hope is that some of the developers here will try the patch an report their results.
I believe that the abstraction offered by the taskset concept has value on it's own. If the performance of my implementation isn't found to be as good or better than the locking from before, I will implement a version that uses the old locking scheme. A given port/variant of HandBrake could then select the appropriate taskset backend for it. This would support my eventual goal of adding and supporting a FreeBSD port of HandBrake from within the HandBrake SVN tree.
http://pastebin.ca/1817937
The code adds the concept of a taskset: a group of cooperating threads, working on the same task. The taskset API encapsulates all of the tedium of managing the threads and synchronization primitives. A consumer of taskset, need only create, cycle for each unit or work, and destroy the taskset in order to complete the task. The threads of the task are provided with two simple functions to wait for work and to signal work completion. The use of the API allows for a ~200 line reduction in decomb.c alone.
When this was last discussed, concerns were raised about performance. Since the original implementation does not run on FreeBSD, I do not have before and after comparisons. I have access to Linux VMs on a Xen system, and I can run before and after tests there, but I fear that the introduction of virtualization could make the results unreliable. My hope is that some of the developers here will try the patch an report their results.
I believe that the abstraction offered by the taskset concept has value on it's own. If the performance of my implementation isn't found to be as good or better than the locking from before, I will implement a version that uses the old locking scheme. A given port/variant of HandBrake could then select the appropriate taskset backend for it. This would support my eventual goal of adding and supporting a FreeBSD port of HandBrake from within the HandBrake SVN tree.
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
After I fixed an initialization problem in decomb.c, this patch worked wonderfully.
http://handbrake.fr/pastebin/pastebin.php?show=1284
You dropped an initialization of pv->yadif_arguments.dst = NULL
It improves speed signficantly. On linux using the equivalent to x264's ultrafast preset:
For reference, ultrafast with no filters is 400 fps
http://handbrake.fr/pastebin/pastebin.php?show=1284
You dropped an initialization of pv->yadif_arguments.dst = NULL
It improves speed signficantly. On linux using the equivalent to x264's ultrafast preset:
Code: Select all
filter fps orig fps taskset
----------- ---------- ----------
decomb 140 177
deint slow 115 258
deint slower 98 240
Re: [PATCH] POSIX Compliant "TaskSet" locking
It looks like (based on a search through your diffs) that I made a similar mistake in rotate.c. I won't be in a place to submit a new set of diffs for a few hours, so if you don't mind publishing a fix for this too, please do.
Did you visually compare the decomb/deinterlace results of the two runs? I just want to double check that the performance jump is not a side effect of some bug I've introduced.
Thanks for testing out the patch!
Did you visually compare the decomb/deinterlace results of the two runs? I just want to double check that the performance jump is not a side effect of some bug I've introduced.
Thanks for testing out the patch!
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
This fixes the rotate initialization.
http://handbrake.fr/pastebin/pastebin.php?show=1286
But decomb and deinterlace are not working right. Looks as if slices from multiple frames are being rendered to output frames.
http://handbrake.fr/pastebin/pastebin.php?show=1286
But decomb and deinterlace are not working right. Looks as if slices from multiple frames are being rendered to output frames.
Re: [PATCH] POSIX Compliant "TaskSet" locking
One problem is that pthread_cond_wait() can suffer from spurious wakeups on some platforms. See http://www.opengroup.org/onlinepubs/007 ... _wait.html for details. FreeBSD doesn't seem to do this, but obviously Linux does. Can you try this updated patch?
http://pastebin.ca/1852546
http://pastebin.ca/1852546
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
Checking the condition again after cond_wait is the right thing to do, but in this case, that wasn't the problem. It's a problem in bit_nclear. The C standard says it is undefined to shift by negative values or values >= wordsize. You we shifting by wordsize in some cases. Here's the fix http://handbrake.fr/pastebin/pastebin.php?show=1292
It now generates correct output for me and the performance looks to be the same as before.
It now generates correct output for me and the performance looks to be the same as before.
Re: [PATCH] POSIX Compliant "TaskSet" locking
I fixed another instance of the "shift by word size or larger" and have regenerated the patch set. Nice catch!
http://pastebin.ca/1853216
--
Justin
http://pastebin.ca/1853216
--
Justin
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
Um, where is the other one. I don't see it.
And the change you made to how I was doing it is good, but you reverted an occurrence of the problem that I had fixed
(stop_pos & 0x1f) + 1 can be 32.
And the change you made to how I was doing it is good, but you reverted an occurrence of the problem that I had fixed
Code: Select all
+ | ( 0xFFFFFFFF << ( ( stop_pos & 0x1F ) + 1 ) ) );
Re: [PATCH] POSIX Compliant "TaskSet" locking
The one I thought you missed, you properly fixed in the previous patch, I just missed it when parsing the full file diff. I also missed your fixes for the left shift operators. Here's another attempt, again using the '0 bit in mask' trick to avoid the extra shift operation.
http://pastebin.ca/1853308
Just to make it easier to see, this is what bit_nclear() now looks like.
http://pastebin.ca/1853308
Just to make it easier to see, this is what bit_nclear() now looks like.
Code: Select all
static inline void
bit_nclear(uint32_t *bit_map, int start_pos, int stop_pos)
{
int start_word = start_pos >> 5;
int stop_word = stop_pos >> 5;
if ( start_word == stop_word )
{
bit_map[start_word] &= ( ( 0x7FFFFFFF >> ( 31 - (start_pos & 0x1F ) ) )
| ( 0xFFFFFFFE << ( stop_pos & 0x1F ) ) );
}
else
{
bit_map[start_word] &= ( 0x7FFFFFFF >> ( 31 - ( start_pos & 0x1F ) ) );
while (++start_word < stop_word)
bit_map[start_word] = 0;
bit_map[stop_word] &= 0xFFFFFFFE << ( stop_pos & 0x1F );
}
}
Re: [PATCH] POSIX Compliant "TaskSet" locking
Any update on the FreeBSD port?
I'd love some dummy friendly compilation instructions to get Handbrake compiled on FreeBSD/AMD64.
I'd love some dummy friendly compilation instructions to get Handbrake compiled on FreeBSD/AMD64.
Re: [PATCH] POSIX Compliant "TaskSet" locking
This stuff is overdue for being committed, I just haven't had an opportunity yet to test the code and integrate it with filter changes that have occurred in the intervening time. I really am sorry scisguy, and it really is at the top of my todo list.
Re: [PATCH] POSIX Compliant "TaskSet" locking
*Bump*
Any update on getting this into the FreeBSD port?
Any update on getting this into the FreeBSD port?
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
We haven't forgotten about it.
Re: [PATCH] POSIX Compliant "TaskSet" locking
Has there been any progress on this? Nix is good BSD is better. Sorry for the bump.
Re: [PATCH] POSIX Compliant "TaskSet" locking
Not quite yet. Not many folk have much time to work on the project these days, so things are slow going. Hopefully won't be too long now that the release isn't blocking things.
Re: [PATCH] POSIX Compliant "TaskSet" locking
Pushing this thread up to the top of the priority list given it's been about for a while and not submitted yet.
http://handbrake.fr/pastebin/pastebin.php?show=1292 still applies. The patches you put on pastebin.ca seem to have disappeared (pastebin.ca seems to be down) so the above seems to be the latest.
Started doing some new encode tests with this applied to the trunk. No problems so far. Will post results later.
http://handbrake.fr/pastebin/pastebin.php?show=1292 still applies. The patches you put on pastebin.ca seem to have disappeared (pastebin.ca seems to be down) so the above seems to be the latest.
Started doing some new encode tests with this applied to the trunk. No problems so far. Will post results later.
Re: [PATCH] POSIX Compliant "TaskSet" locking
I merged this with pastebin 1292: http://paste.handbrake.fr/pastebin.php?show=2163scsiguy wrote:The one I thought you missed, you properly fixed in the previous patch, I just missed it when parsing the full file diff. I also missed your fixes for the left shift operators. Here's another attempt, again using the '0 bit in mask' trick to avoid the extra shift operation.
http://pastebin.ca/1853308
Just to make it easier to see, this is what bit_nclear() now looks like.
Code: Select all
static inline void bit_nclear(uint32_t *bit_map, int start_pos, int stop_pos) { int start_word = start_pos >> 5; int stop_word = stop_pos >> 5; if ( start_word == stop_word ) { bit_map[start_word] &= ( ( 0x7FFFFFFF >> ( 31 - (start_pos & 0x1F ) ) ) | ( 0xFFFFFFFE << ( stop_pos & 0x1F ) ) ); } else { bit_map[start_word] &= ( 0x7FFFFFFF >> ( 31 - ( start_pos & 0x1F ) ) ); while (++start_word < stop_word) bit_map[start_word] = 0; bit_map[stop_word] &= 0xFFFFFFFE << ( stop_pos & 0x1F ); } }
That way we should have an up-to-date (I hope) version for reference.
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
For people interested in this, the patch is being tracked and kept up to date here:
https://reviews.handbrake.fr/r/185/
https://reviews.handbrake.fr/r/185/
- JohnAStebbins
- HandBrake Team
- Posts: 5726
- Joined: Sat Feb 09, 2008 7:21 pm
Re: [PATCH] POSIX Compliant "TaskSet" locking
After more than 2 years, finally committed.
https://trac.handbrake.fr/changeset/4685
Could have been worse. Bob took 3 years
https://trac.handbrake.fr/changeset/4685
Could have been worse. Bob took 3 years