转载

代码审查关注什么：性能

在本系列代码审查文章的第三篇，我们准备讨论在代码审查中性能方面需要关注哪些事情。

和所有的架构/设计一样，一个系统非功能性的性能需求也应该优先考虑。不管你是在开发必须在几个毫微秒内响应的低等待时间交易系统、还是在创建一个需要尽快响应用户的购物网站，或者是在开发一款管理 “待办事项”的移动应用，你都应该有“太慢了”的概念。

我们一起来看看审查者在代码审查在中应该关注的影响性能的事情。

首先

这个功能有硬性的性能需求吗？

被审查的代码属于有公开的 SLA领域吗？或者说需求描述中有单独的性能方面的需求？

如果最初的需求是来自 bug 描述“登录界面加载太慢”，开发者应该已经很清楚多长的加载时间是合适的，否则审查者或作者怎么相信速度有明显的提高？

如果是这样，有测试证明达到这些标准了吗？

任何性能关键系统都应该有自动化性能测试，保证达到公开的 SLAs （“所有的订购请求都应该在10ms内响应”）标准。如果过没有，你只有依靠用户来投诉达不到你的 SLAs 标准。这不仅仅是糟糕的用户体验，还会导致不可避免的罚款和费用。在上一篇文章中已经详细讨论了审查测试。

bug 修复/新功能添加的代码会对已有的性能测试带来负面影响吗？

如果你们定期运行性测试（或者如果你们有一套性能测试程序可以按需运行），确保新的代码在性能标准方面没有降低系统性能。这可能是一个自动化的过程，但是在 CI 环境中运行性能测试远不如运行单元测试常见，所以在审查过程中将这一点提出来是很有必要的。

如果本次代码评审没有硬性的性能要求

那么花数小时时间极其痛苦的优化代码最终就节约了几个 CPU 周期就完全没有必要。但是代码审查者也需要确保代码不要掉进一些完全可以避免的性能问题中。检查列表的其他部分，看看是否可以轻易的阻止未来的性能问题。

调用外部服务/应用开销很高

使用你自己应用以外任何需要网络的系统都会比一个糟糕的equals()方法消耗的时间要多的多。考虑：

调用数据库

最糟糕的可能是像 ORM 这种隐藏在抽象后面的调用。但是在代码审查中你应该能够发现普通的性能问题根源，像在循环中单独调用数据库--例如，加载一个 ID 列表，然后根据 ID 为每一项单独查询数据库。

代码审查关注什么：性能

例如，第19行也许看起来没什么问题，但是它处在一个 for 循环中--你不知道这段代码会调用多少次数据库。

不必要的网络访问

像数据库一样，远程服务常常也是因为多个远程调用而过度使用，其实一次调用可能就足够了，或者批量访问或者缓存处理也许就能阻止昂贵的网络调用。再次说明，和数据库一样，有时一些封装后的方法隐藏了其背后实际是调用一个远程 API。

移动应用/穿戴式应用过多的调用后端服务

这其实和“不必要的网络服务”是一样的，但是在移动设备上，调用后端接口不仅仅是性能问题，还会消耗设备的电量。

高效的使用资源

就和怎么使用网络资源一样，审查者要检查对其他资源的使用，确认是否可能存在性能问题。

代码在访问共享资源时使用锁了吗？这会导致性能低下或者死锁吗？

锁就是性能杀手，并且在多线程环境中很难找到原因。考虑这样的模式：只有一个线程写/修改值，其他线程只能读取，或者使用 lock free 算法。

代码中存在内存泄漏的可能吗？

在 Java 中，导致内存泄漏的常见原因有：可变的静态属性，使用 ThreadLocal 和使用 ClassLoader。

内存的应用消耗有可能无限增长吗？

这一点和内存泄漏不同--内存泄漏是未使用的对象无法被回收。但是任何语言，即使是没有使用垃圾回收机制的语言，都会创建出无限增长的数据结构。作为审查者，如果发现新的值持续被添加到一个 list 或者 map，问一问这个 list 或者 map 有没有清空或者调整大小，什么时候清空或者调整。

代码审查关注什么：性能

在上面的代码中，我们看到所有来自 Twitter 的消息都被添加到一个 map 中。如果我们仔细检查这个类，就会发现 allTwitterUsres 这个 map 从来就没有删减过，TwitterUsers 这个 list 也是一样。这个 map 会迅速变得很庞大，这取决于我们关注了多少用户以及添加 tweets 的频度。

代码关闭连接/流了吗？

写代码时很容易忘记关闭连接或者文件流/网络流。当你在审查其他人的代码时，不管它以什么语言实现，如果有使用文件、网络连接或者数据库连接，请确保它已经正确的关闭。

代码审查关注什么：性能

原作者很容易忽略这个问题，就如上面的这段代码，编译不会有任何问题。作为审查者，你应该发现在函数退出之前，connection，statement和 Result set 都需要关闭。在 Java 7中，由于可以使用 try-with-resources 使得这一点更容易管理。下面的截图展示了作者使用 try-with-resource 修改代码后的结果。

代码审查关注什么：性能

资源池正确的配置了吗？

一个环境可选的配置项依赖于很多因素，所以并不是审查者立刻能够知道。例如数据库连接池的大小是否配置正确。但是有些要点是你一眼就能发现的，例如池子是否太小（如大小为1）或者太大（数百万线程）。很好的调节这些值需要一个尽可能真实的测试环境。但是在代码审查中可以辨别的常见问题是池子（例如线程池、连接池）太大。逻辑上说当然是越大越好，但是每一个对象都要占用资源，并且管理数千个项目的开销要远远高于从它们当中获取的好处。如果有疑问，默认值通常是不错的选择。代码配置如果和默认值不一致，应该有测试或者结论说明设定的值更合理。

审查者很容易发现的警告信号

有一些代码会引入潜在的性能问题。这取决于所使用的代码和库。

反射

在 Java 中使用反射要比不使用反射慢。如果你正审查的代码中使用了反射，问一下是否真的需要。

代码审查关注什么：性能

上面的截屏展示了审查者在 Upsource 中点击一个方法查看它来自哪里，你会看到这个方法返回了java.lang.reflect包里的某个类型，这应该是一个警告信号。

超时

当你在审查代码时，你也许不知道一个操作合理的超时是多少，但是你应该考虑“当超时在倒计时时，对系统的其他方面会议什么影响？”。作为审查者你应该考虑最坏情况-在5分钟的超时时间内系统会阻塞掉吗？如果这个时间设置的是1秒最坏会发生什么情况？如果代码的作者不能为说明为什么选定超时的长度，作为审查者你也不知道选定的超时长度的优缺点，这个时候需要邀请知道这个超时影响的人来讨论。不要等待用户来投诉性能方面的问题。

并行

代码使用了多个线程来完成一个简单任务？使用多线程是添加了时间和复杂性而不是提高性能？在最新的 Java 中，这可能比直接创建一个线程更隐蔽：代码中使用了 Java 8最新的并发 stream 但是并没有享受到并发的好处？例如：使用并发 stream 处理少量的数据，或者使用 stream 处理一个很简单的任务，有可能比使用串行 stream 处理更慢。

代码审查关注什么：性能

在上面的例子中，新增加的 parallel 调用好像没有给我吗带来任何好处--这里 stream 作用于一条 Tweet，也就是最多140个字符的字符串。将并发应用到这么短的字符上可能不会带来任何性能的提高，相反将字符串分割到各个并发线程所付出的代价要比并发带来的好处要高。

正确性

这些事情不一定会影响系统的性能，但是由于好多线程环境强相关，所以它们和性能这个主题相关。

代码中为多线程选择了正确的数据结构？

代码审查关注什么：性能

在上面的代码中，在第12行使用了一个 ArrayList来存储所有的 sessions。但是这段代码会被多个线程调用，尤其是 onOpen 方法，所以 sessions 应该使用一个线程安全的数据结构。在这个例子中，我们有很多选择：可以使用 Vector，也可以使用 Collections.synchronizedList() 来创建一个线程安全的 List，但是最好的选择可能是 CopyOnWriteArrayList，因为这个 list 被修改的频率要远远低于被读取的频率。

代码审查关注什么：性能