在编写代码的时候,我们往往要在多方面因素之中进行权衡。正确性毫无疑问是最基本的要求,这个都不需要讨论。而如何在可读性和性能之间进行权衡,确实是值得思考的问题。
我对于这方面的看法是:我更倾向于可读性为第一位。只有当代码的可读性好的时候,才容易维护,且不容易引起bug。并且当真的需要提高性能的时候,由于代码的良好的可读性,才便于分析代码,找出瓶颈,从而提高性能。
那么在不影响可读性的基础上,如何提高代码的性能呢?我认为第一步要做的是,审视系统架构,从软件到硬件,寻找可以进行提高的地方。比如,是否可以很好的支持并发,等等。第二,审视软件设计,寻找处理瓶颈,进行设计上的提高;第三,审视使用的算法,优化算法,提高性能。最后,在以上都无可作为的情况下,只能进行代码级别的优化时,一定要通过性能测试或者分析如profile等,找到真正的热点或者瓶颈。不然费了半天的劲,却只是事倍功半。而且在等进行代码级别的优化时,也要慎重优化,仔细的考量是否性能的提升程度,是否值得降低代码的可读性或者可维护性。是否有其它更好的方法解决。绝对不能以高性能作为丑陋代码的借口!
下面谈谈我这两天解决的一个Bug,也是引起我写这篇文章的原因。该Bug的现象是将某一型号的设备升级到mainline时,有时候核心进程会不断的crash。该模块不是我负责的模块,所以查了1天才找到问题。看到造成问题的代码,我实在是费解。
代码样例如下:
- void opt_hash_index(u32 *index)
- {
- switch (g_mem_size) {
- case HIGH_MEM_SIZE:
- *index = *index & HIGH_MEM_MASK;
- break;
- case MEDIUM_MEM_SIZE:
- *index = *index & MEDIUM_MEM_MASK;
- break;
- case LOW_MEM_SIZE:
- *index = *index & LOW_MEM_MASK;
- break;
- case LOWEST_MEM_SIZE:
- *index = *index & LOWEST_MEM_MASK;
- break;
- default:
- break;
- }
- }
我在看到这个函数的时候,看到opt_hash_index是根据四种不同情况,才对index进行修改。最初的想法是该index只有在这四种情况下,才需要进行修改,其余情况则不需要修改。由于没有coredump文件,只有崩溃时的函数栈,且bug很难重现,所以我只能通过阅读代码来查找问题。但是在我看完计算index的hash算法后,发现该函数并没有保证index的大小一定小于桶的个数。
于是又重新查看opt_hash_index这个函数,查看各个case值和mask的宏定义:
- #define HIGH_MEM_CNT 20
- #define MEDIUM_MEM_CNT 19
- #define LOW_MEM_CNT 18
- #define LOWEST_MEM_CNT 17
-
- #define HIGH_MEM_MASK ((1ul<<HIGH_MEM_CNT)-1)
- #define MEDIUM_MEM_MASK ((1ul<<MEDIUM_MEM_CNT)-1)
- #define LOW_MEM_MASK ((1ul<<LOW_MEM_CNT)-1)
- #define LOWEST_MEM_MASK ((1ul<<LOWEST_MEM_CNT)-1)
-
- #define HIGH_MEM_SIZE (1ul<<HIGH_MEM_CNT)
- #define MEDIUM_MEM_SIZE (1ul<<MEDIUM_MEM_CNT)
- #define LOW_MEM_SIZE (1ul<<LOW_MEM_CNT)
- #define LOWEST_MEM_SIZE (1ul<<LOWEST_MEM_CNT)
于是我修改了opt_hash_index函数,直接使用求模运算来限制index的大小。
- void opt_hash_index(u32 *index)
- {
- *index %= g_mem_size;
- }
当我得知这个原因时,真有些发疯了。这样的代码可读性和维护性简直是一塌糊涂啊!如果你只支持那四个固定的资源个数。拜托请加上一些注释,并且在default处使用BUG()开关,使代码运行到此处自动crash或者报警才是!再说了为了这一点点的性能提升,放弃了这么多,值的吗?!
于是我想看看switch..case究竟比mod能提高多少性能。毕竟switch case的实现一般都是使用跳转完成的,那么额外的比较也需要cpu的计算资源,更何况跳转会破坏CPU的pipeline,即流水线工作呢!
于是我写了下面的简单的测试程序:
- #define LOOPS 100000000
-
- int main(void)
- {
- int i;
-
- g_mem_size = LOW_MEM_SIZE;
- for (i = 0; i < LOOPS; ++i){
- opt_hash_index1(i);
- }
-
-
- return 0;
- }
在使用switch case的情况下:
1. g_mem_size = HIGH_MEM_SIZE:0.413s
2. g_mem_size = MEDIUM_MEM_SIZE: 0.447s
3. g_mem_size = LOW_MEM_SIZE: 0.378s
4. g_mem_size = LOWEST_MEM_SIZE: 0.345s
使用mod运算时,跟g_mem_size大小无关,大约为0.403s
OK。switch case总体上是比mod这种方案性能要好一些。但是这是在循环1亿次的情况下,最快才提高不过0.06s。就这点性能的提升,值的写出这样天人共愤的代码!
我不想在这种性能优化与可读性的观点与其过多的争执。既然想要提升性能,那么我照样有好的方法,稍稍的降低一点可读性。
- void opt_hash_index3(u32 index)
- {
- index &= (g_mem_size-1);
- }
测试一下性能,其与g_mem_size的大小无关,约0.345s左右。
于是我今天回复美国那边,我不能接受以前的方案,可读性和维护性太差了!既然追求高性能,那么我可以使用方案三来取代mod运算。我将性能测试结果也告诉了他。方案三的性能既比switch case要好,可读性也要强得多。看看明天他如何回复。
其实,我为什么在开始的时候,直接使用mod而没有用方案三呢?还是因为可读性,执行一亿次才相差这么点,不值得牺牲可读性。
退一万步讲,即使有不错的性能提升,但是这种性能的提升也绝不是这种丑陋代码的遮羞布!完全可以使用各种方法来避免这种离奇的switch case。对此我不一一举例了。
我认为只有优美的代码才能保证质量,才能进行性能的提升!不要拿提高性能来为丑陋的代码遮羞!它不是一块遮羞布!!!