在本系列代码审查文章的第三篇,我们准备讨论在代码审查中性能方面需要关注哪些事情。
和所有的架构/设计一样,一个系统非功能性的性能需求也应该优先考虑。不管你是在开发必须在几个毫微秒内响应的低等待时间交易系统、还是在创建一个需要尽快响应用户的购物网站,或者是在开发一款管理 “待办事项”的移动应用,你都应该有“太慢了”的概念。
我们一起来看看审查者在代码审查在中应该关注的影响性能的事情。
首先
这个功能有硬性的性能需求吗?
如果最初的需求是来自 bug 描述“登录界面加载太慢”,开发者应该已经很清楚多长的加载时间是合适的,否则审查者或作者怎么相信速度有明显的提高?
如果是这样,有测试证明达到这些标准了吗?
bug 修复/新功能添加的代码会对已有的性能测试带来负面影响吗?
如果你们定期运行性测试(或者如果你们有一套性能测试程序可以按需运行),确保新的代码在性能标准方面没有降低系统性能。这可能是一个自动化的过程,但是在 CI 环境中运行性能测试远不如运行单元测试常见,所以在审查过程中将这一点提出来是很有必要的。
如果本次代码评审没有硬性的性能要求
那么花数小时时间极其痛苦的优化代码最终就节约了几个 CPU 周期就完全没有必要。但是代码审查者也需要确保代码不要掉进一些完全可以避免的性能问题中。检查列表的其他部分,看看是否可以轻易的阻止未来的性能问题。
调用外部服务/应用开销很高
使用你自己应用以外任何需要网络的系统都会比一个糟糕的equals()
方法消耗的时间要多的多。考虑:
调用数据库
最糟糕的可能是像 ORM 这种隐藏在抽象后面的调用。但是在代码审查中你应该能够发现普通的性能问题根源,像在循环中单独调用数据库--例如,加载一个 ID 列表,然后根据 ID 为每一项单独查询数据库。
例如,第19行也许看起来没什么问题,但是它处在一个 for
循环中--你不知道这段代码会调用多少次数据库。
不必要的网络访问
像数据库一样,远程服务常常也是因为多个远程调用而过度使用,其实一次调用可能就足够了,或者批量访问或者缓存处理也许就能阻止昂贵的网络调用。再次说明,和数据库一样,有时一些封装后的方法隐藏了其背后实际是调用一个远程 API。
移动应用/穿戴式应用过多的调用后端服务
这其实和“不必要的网络服务”是一样的,但是在移动设备上,调用后端接口不仅仅是性能问题,还会消耗设备的电量。
高效的使用资源
就和怎么使用网络资源一样,审查者要检查对其他资源的使用,确认是否可能存在性能问题。
代码在访问共享资源时使用锁了吗?这会导致性能低下或者死锁吗?
代码中存在内存泄漏的可能吗?
在 Java 中,导致内存泄漏的常见原因有:可变的静态属性,使用 ThreadLocal 和使用 ClassLoader
。
内存的应用消耗有可能无限增长吗?
这一点和内存泄漏不同--内存泄漏是未使用的对象无法被回收。但是任何语言,即使是没有使用垃圾回收机制的语言,都会创建出无限增长的数据结构。作为审查者,如果发现新的值持续被添加到一个 list 或者 map,问一问这个 list 或者 map 有没有清空或者调整大小,什么时候清空或者调整。
在上面的代码中,我们看到所有来自 Twitter 的消息都被添加到一个 map 中。如果我们仔细检查这个类,就会发现 allTwitterUsres
这个 map 从来就没有删减过,TwitterUsers
这个 list 也是一样。这个 map 会迅速变得很庞大,这取决于我们关注了多少用户以及添加 tweets 的频度。
代码关闭连接/流了吗?
写代码时很容易忘记关闭连接或者文件流/网络流。当你在审查其他人的代码时,不管它以什么语言实现,如果有使用文件、网络连接或者数据库连接,请确保它已经正确的关闭。
资源池正确的配置了吗?
一个环境可选的配置项依赖于很多因素,所以并不是审查者立刻能够知道。例如数据库连接池的大小是否配置正确。但是有些要点是你一眼就能发现的,例如池子是否太小(如大小为1)或者太大(数百万线程)。很好的调节这些值需要一个尽可能真实的测试环境。但是在代码审查中可以辨别的常见问题是池子(例如线程池、连接池)太大。逻辑上说当然是越大越好,但是每一个对象都要占用资源,并且管理数千个项目的开销要远远高于从它们当中获取的好处。如果有疑问,默认值通常是不错的选择。代码配置如果和默认值不一致,应该有测试或者结论说明设定的值更合理。
审查者很容易发现的警告信号
有一些代码会引入潜在的性能问题。这取决于所使用的代码和库。
反射
上面的截屏展示了审查者在 Upsource 中点击一个方法查看它来自哪里,你会看到这个方法返回了java.lang.reflect
包里的某个类型,这应该是一个警告信号。
超时
当你在审查代码时,你也许不知道一个操作合理的超时是多少,但是你应该考虑“当超时在倒计时时,对系统的其他方面会议什么影响?”。作为审查者你应该考虑最坏情况-在5分钟的超时时间内系统会阻塞掉吗?如果这个时间设置的是1秒最坏会发生什么情况?如果代码的作者不能为说明为什么选定超时的长度,作为审查者你也不知道选定的超时长度的优缺点,这个时候需要邀请知道这个超时影响的人来讨论。不要等待用户来投诉性能方面的问题。
并行
代码使用了多个线程来完成一个简单任务?使用多线程是添加了时间和复杂性而不是提高性能?在最新的 Java 中,这可能比直接创建一个线程更隐蔽:代码中使用了 Java 8最新的并发 stream 但是并没有享受到并发的好处?例如:使用并发 stream 处理少量的数据,或者使用 stream 处理一个很简单的任务,有可能比使用串行 stream 处理更慢。
在上面的例子中,新增加的 parallel
调用好像没有给我吗带来任何好处--这里 stream 作用于一条 Tweet, 也就是最多140个字符的字符串。将并发应用到这么短的字符上可能不会带来任何性能的提高,相反将字符串分割到各个并发线程所付出的代价要比并发带来的好处要高。
正确性
这些事情不一定会影响系统的性能,但是由于好多线程环境强相关,所以它们和性能这个主题相关。
代码中为多线程选择了正确的数据结构?
代码是否容易产生竞争条件?
在多线程环境中,非常容易写出导致竞争条件的代码。例如:
正确的使用锁了吗?
为代码添加的性能测试是否有用?
因为很容易写出很差的测试代码。或者测试用例中使用的数据和真实数据完全不一样,这可能会给出误导我们的结果。
缓存
使用缓存是阻止过多的外部请求的一种方式,同时它自身也会带来问题。如果你审查的代码中使用了缓存,你应该关注一些常见问题,比如:缓存项目的有效处理不正确。
代码级别的优化
如果你是代码审查者,同时又是开发者,接下来的这一节也许有你喜欢建议的优化方法。作为一个团队,你需要知道性能对你有多么重要,以及这些类别的优化对你的代码是否有用。
- 代码中是否使用了不必要的同步/锁?如果代码总是在单线程上运行,锁就是不必要的开销。
- 代码中是否使用了不必要的线程安全数据结构?例如:
Vector
是否可以用ArrayList
来替换? - 代码中是否使用常用操作性能很差的数据结构?例如:使用了单向链表,但是需要经常在其中搜索一个单项?
- 代码使用懒加载是否会更好?
- 是否将
if
语句或者其他可以短路的逻辑判断放在最前面? - 是否使用了很多字符串格式化?可以更高效的完成吗?
- log语句是否使用了字符串格式化?有提供给
if
语句根据 log 级别判断是否输出?
上面的代码只输出级别为 FINEST 的信息。但是开销昂贵的字符串格式化每次都会执行,不管消息是否输出。
像上面的代码这样修改可以提高性能,只有需要输出的消息才格式化。
在 Java 8 中,不使用 if
这样的模板代码也可以获得这些性能。由于使用了 lambda 表达式,字符串格式化操作只有在 log 输出时才会执行。这应该是 Java 8 中最好的方法。
在考虑性能问题时要担心的问题非常多。。。
尽管本文是针对所有的开发者,很多例子是特别针对 Java/JVM。我还是想用 Java 代码审查者几条简单的建议来结束本文,这样可以让 JVM 来优化你的代码而不是你自己亲自来优化:
- 方法和类尽量小
- 逻辑尽量简单--没有很深的
if
判断或者循环
结论
在考虑性能问题时,需要记住:有的问题在代码审查阶段界需要解决(例如:不必要的数据库访问),有的问题暂时提交评论就可以(如代码级别的优化),因为这些事情可能不会给你的系统带来足够的价值。