I’m still wandering through xine’s sources to apply my own style to them, hoping that I can spot problematic areas before they become a problem. It’s quite interesting what I’ve seen, as the code is really suboptimal in some cases: functions could be factored out so that they are shared between plugins, in some we have leaks because we allocate stuff and then return without the object, in others we could reduce the size of a code section protected by a mutex.
I haven’t committed and pushed these changes yet because I’m sure it will be a mess to merge them into 1.2, so I’ll first try that when I’m willing to spend time to merge them back into 1.2.
In general, what I’ve been following for xine’s source changes are a few simple rules:
- whenever possible, use constants instead of variables;
- whenever possible, declare constants and variables only when you can give them a meaningful value;
- whenever possible, reduce the scope of variables and constant by adding blocks;
- in each function, test first all the prerequisites, before any allocation or constant calculation;
- in each function, when a condition has to be checked and no code has to be executed if it’s not true, return from the function rather than increasing the indentation level with a new
ifblock; - if the same exact calculation has to be done on different values, create a function, even if it’s a single return or little more; constant functions will most likely be optimised by the compiler;
- when a function require using mutexes, make sure that allocations and freeing of memory areas happen outside the mutex-protected code, even if that means keeping memory allocated for a while longer than necessary.
Unfortunately xine, while depending on a C99-compliant compiler already – for FFmpeg – does not require enabling all the C99 features yet. Which means I can’t use C99-style for loops, which I found quite useful to make code clean.
My reasoning for these rules are quite easy actually.
Assigning the value to a variable when it’s declared makes unused variable warnings from GCC more useful (assignment is considered use, which means that a variable declared and then assigned is never considered unused – which by the way is wrong, GCC should warn in that case too, but I’m no good with compilers so I can’t patch it for this), and makes it less likely that a quick change makes use of the variable before a value is assigned.
Marking data constant whenever possible makes it less likely that the meaning of a variable is changed. I find it nicer to have ten constants than one variable with ten different meanings. It also often forces to get more meaningful names for the data instead of relying on variables called tmp, n, i, j, k…
Reducing the scope of variables combine the two objectives above: you cannot abuse a variable if its scope is limited, and you cannot use the variable before it gets an useful value if it’s not in your scope.
Checking conditions beforehand makes it possible to avoid allocations before prerequisites are checked, which in turn makes it possible to avoid allocation and freeing cycles. It also makes it possible to avoid locking an unlocking mutexes for no good reason.
Returning earlier from a function is instead useful to avoid multiple indentation levels in case multiple prerequisites are checked in series. It’s just readability that gets improved, but still it’s a lot.
Splitting a calculation makes it less likely to make a copy-paste mistake in the code, and it makes it easier to change it if it needs fixes.
And mutex-protected code should be as limited as possible as it’s code that doesn’t make good use of multicore systems, and in today’s computers world that’s bad.
I think I’ll complete this cleanup soon enough, and then move to 1.2 to change the paradigm a bit more. Mostly I’m tempted to see where we can replace some mutexes with R/W locks (which should make xine work better in parallel, as thread would block each other only while writing).
I also start thinking of a few things we could try to use for 1.3 or 2.0, namely glib’s lists and thread supports to replace xine’s own implementations. My reason for this is that it makes very little sense to reinvent the wheel for these things when they are already well developed and available.
Anyway, I sincerely hope to make some difference in the not-so-distant future, but I don’t count on it just yet. I would really like to see how my changes applies to multicore system, and non-x86 systems. Too bad Sun boxes cost so much, or I’d be glad to work with an 8-way UltraSPARC CPU (and Solaris maybe) and optimise xine for it. If somebody from Sun is reading, a developer discount program would be appreciated 😉