代码审查“查”什么(3)：性能

　　英文原文：What to look for in a Code Review: Performance

　　在代码审查系列的第三篇，我们将涉及就性能而言审查代码时需要留意什么。

　　和所有的架构或设计领域一样，一个系统性能的非功能性要求应该在前期就确定下来。无论你是工作在一个响应要求在纳秒级别的低延迟交易系统上，还是创建一个需要响应客户的在线商店，或者正在编写一个电话应用去管理 “待办事项”列表，对“太慢”都要有相应的认识（译者注：环境不同，含义不同）。

　　让我们来讨论一些影响性能的事情，审查者可以在代码审查时留意。

　　首先

　　这部分功能有硬性的性能需求吗？

　　这部分审查的代码是否属于一个以前发布的服务层级协议（Service Level Agreement SLA）？或者在需求中有规定必需的性能指标吗？

　　如果最初的需求来自于一个bug —— “登录界面加载太慢”，最初的开发者应该搞清楚一个合理的加载时间是多少 —— 否则审查者或者作者如何相信这个优化后的速度满足需求？

　　如果有，有测试去检查吗？

　　任何对性能敏感的系统都应该有自动化测试，来保证可以满足公布的 SLA（在 10 ms 内为所有订单需求提供服务）。如果没有这些，你就只有等到客户投诉时才知道你没有满足 SLA。 这不仅让用户感觉很糟，而且还会导致了不必要的罚款和费用。在这系列的上一篇里，已经详细讨论了代码审查的测试部分。

　　功能修改或新功能对已有性能测试结果有不良影响吗？

　　如果你会定期执行性能测试（或者你有一套可以按需要执行的测试集），应该检查对性能有严格要求部分的新代码，确保它不会造成系统性能下降。 这可以是一个自动化的流程，但性能测试通常不像单元测试那么普遍地被集成到 CI 环境中，所以值得强调这一审查步骤。

　　代码审查中没有硬性的性能需求要怎么做？

　　如果经过数小时的痛苦，只节省了一点点 CPU 周期，这样的优化实在划不来。但有些事情如果审查者检查了，就可以保证代码不会存在常见的性能缺陷 （完全可以避免）。检查列表的其他部分，看是不是有简单的方法可以防止未来可能出现的性能问题。

　　服务或应用外的调用开销很大

　　在自己的应用之外使用需要跨网络跃点的系统和使用一个优化很差的 equals() 方法相比，前者开销更大。请考虑这些：（译者注：网络跃点 network hop 即路由。一个路由为一个跃点。传输过程中需要经过多个网络，每个被经过的网络设备点（有能力路由的）叫做一个跃点，地址就是它的 IP）

　　数据库调用

　　最大的麻烦可能躲在抽象后面，比如 ORM（Object Relational Mapping，对象关系映射）。但在代码审查时，你应该能够找到性能问题的常见原因，就像一个循环里的多次数据库调用 —— 例如 ，加载一个 ID 列表，然后基于每个 ID 查询数据库里的对应项目。

　　例如 ，19 行看起来没有什么问题（无辜的），但它在一个 for 循环里 —— 你不知道这行代码会导致多少次数据库调用。

　　不必要的网络调用

　　如同数据库，有时远程服务也被过度使用。就像有些地方使用了多次远程调用，其实一次就足够了，或者可以用批处理或缓存避免开销巨大的网络调用。另一个和数据库类似的是，一个抽象有时候会隐藏一个实际在呼叫远程 API的方法调用。

　　移动程序、可穿戴程序过于频繁地调用后端

　　这基本上和“不必要的网络调用”类似，但在移动设备上还增加了一个问题，不必要的调用不仅降低性能，而且还很耗电。

　　高效且有效果地使用资源

　　除了注意如何使用网络资源外，审查者还可以查看其他的资源使用，来确定可能的性能问题。

　　代码通过锁来访问共享代码吗？这样会导致性能下降和死锁吗？

　　锁是性能的杀手，特别是在多线程环境下。考虑这样的方式：仅有一个线程写入或改变数值，而其他线程可以任意读取；或者使用无锁算法。

　　代码会造成内存泄漏吗？

　　一些 Java 中的常见原因是：可变静态字段，使用 ThreadLocal 和 ClassLoader。

　　应用程序的内存会无限增加吗？

　　这和内存泄漏不同 —— 内存泄漏是由于垃圾收集器不能回收不再使用的对象。但任何语言，包含那些没有垃圾收集机制的语言，都可以创建无限增长的数据结构。作为审查者，如果你看到新的值不断被加入一个列表或者映射表（Map） ，问一下这个列表或者映射表是否或者何时被释放或者减少。

　　上面的代码中我们可以看到，所有来自 Twitter 的消息都被加入一个映射表。如果我们仔细检查这个类，我们发现 allTwitterUsers 映射表从来没有减少过，TwitterUser中的 tweets 列表也没有。这个映射表可能很快就变得很大，这取决于我们监控用户的数量和我们添加 tweets 的频率。

　　代码有关闭连接或流吗？

　　一不小心就会忘记关闭连接、文件或网络流。当你审查其他人（无论什么语言）的代码时，确保每个使用的文件、网络或数据库连接都有被正确地关闭。

　　上面代码很幸运地编译通过，最初的作者很容易忽视这个问题。作为审查者你应该注意 Connection、Statement 和 ResultSet 这些对象在方法退出前都要关闭。在 Java 7 中通过 try-with-resources 可以很容易做到这一点。下面的截屏显示了作者利用 try-with-resources 修改代码后代码审查的结果。

　　资源池配置正确吗？

　　一个环境的最优配置受很多因素的影响，作为审查者你不可能马上知道，就好像一个数据库连接池的大小是否合适。但是有些事情你可以很容易判断，比如这个池子太小（大小为1）或者太大（数以百万计的线程）。优化这些值需要经过测试，测试环境越接近真实环境越好，但如果池子（例如线程池或者连接池）真的太大，在代码审查时可以抓出这个常见问题。逻辑表明越大越好，但每个对象都会消耗资源，对象太多通常会造成管理的开销比带来的好处大很多。如果有疑问，最好先使用默认设定。没有使用默认设定的代码需要通过某些测试或计算，来验证数值是合理的 。

　　审查者容易发现警告标志

　　某些类型的代码可以立即显示一个可能的性能问题。这取决于使用的语言和函数库（请让我们知道，在你们环境下对“代码异味”的评价）。

　　（译者注：代码异味 code smells，代码中的任何可能导致深层次问题的症状都可以叫做代码异味。代码异味包括重复代码、巨型类、方法过长等。）

　　反射

　　在Java中使用反射比不使用要慢很多。如果你检查包含反射的代码，问一下反射是不是一定需要。

　　上面的截屏显示了一个审查者在 Upsource 里点击一个方法确认它来自哪里，可以看到这个方法来自 java.lang.reflect 包，这就应该是一个警告标志。

　　超时

　　当你审查代码时，你可能不知道一个操作的正常超时应该是多少，但应该这样考虑“如果超时，系统中的其他部分会受到什么影响？”。作为审查者你应该考虑最坏的情况 —— 如果 5 分钟超时到了，应用程序会卡住吗？如果超时被设置为1秒，最坏会发生什么？如果代码的作者无法判断超时的大小，而你作为审查者也不知道如何选择一个值，这时最好让懂行的人参与进来。不要等到你的用户向你抱怨性能问题。

　　并行性

　　代码中有使用多个线程来执行一个简单的操作吗？除了增加时间和复杂性外，却没有带来性能的提高？现代Java中，并行性比直接创建线程更加微妙：代码有使用Java 8 炫酷的并行流机制，但是却没有从并行性中受益吗？例如在并行流中处理非常简单的操作，可能会比在一个顺序流上执行慢很多。

　　上面代码中增加的并行性没有给我们带来任何好处 —— 这个流作用在 Tweet 上，上面一个字符串不会超过140个字符。就把那么几句话的操作并行化，也许不会改善性能，反而拆分成并行线程的开销会更大。

　　正确性

　　这些事情不一定会影响系统的性能，但它们就会影响到正确性，因为它们很大程度上和运行在多线程环境有关。

　　多线程环境下的代码使用了正确的数据结构吗？

　　上面的代码中，作者在第 12 行用一个 ArrayList 存储所有的会话（session）。但这代码特别是 onOpen 方法会被多个线程调用，sessions 字段就必须是一个线程安全的数据结构。这种情况下有多种选择：可以使用Vector，也可以用 Collections.synchronizedList()创建一个线程安全列表（List），但最好的选择是用CopyOnWriteArrayList，因为对这个列表的修改通常远低于对它的读取。

　　代码容易出现竞态条件吗？

　　多线程环境下，编写代码很容易造成微妙的竞争问题。例如：

　　虽然递增代码只有一行（第16 行），但从代码读取值到设定新值的这段时间里，另一个线程很可能已经增加了 orders 的值。作为审查者，要注意 get 和 set 组合都不是原子操作。

　　代码中对锁的使用正确吗？

　　对于竞态条件，作为审查者你应该要检查代码，不允许多个线程用可能会引发冲突的方式修改数值。应该要使用同步、锁或者原子变量来控制修改的代码块。

　　代码的性能测试有价值吗？

　　例如，一不小心就会写出糟糕的微基准测试。或者如果测试使用的数据和量产数据不一样，也会得到一个错误的结果。

　　缓存

　　虽然缓存可以避免过多的外部请求，它也会面临自己的挑战。如果审查的代码使用了缓存，你应该注意一些常见的问题，例如不正确的缓存项失效。

　　代码级优化

　　如果你在审查代码，同时你也是一名开发者，下面的内容中可能会有你想建议的优化方法。作为团队，你们应该很清楚性能对你们有多重要，这些优化方法对你们的代码是否有帮助。

　　大部分公司不会开发低延迟的应用程序，这些优化很可能都属于过早的优化。

• 代码中是否使用了不必要的同步、锁？如果代码总是执行在单线程下，加锁只会带来额外开销。
• 代码中是否使用了不必要的线程安全数据结构？例如，可以用ArrayList 替换 Vector 吗？
• 代码中是否使用了在常见操作上性能很差的数据结构？例如，使用了链表结构，却经常搜索其中的一项。
• 代码是否使用了锁或者同步机制，而实际上可以用原子变量替代？
• 代码可以得益于延迟加载吗？
• if 语句或其他逻辑语句能通过短路机制进行优化吗？比如把最快的计算放在条件的开始？
• 有很多的字符串格式吗？可以更有效率吗？

• log 语句有使用字符串格式化吗？有用检查log级别的 if 语句包起来或者使用的日志提供者可以进行延迟操作？

　　上面代码只会在 logger 被设定为 FINSET 时才会记录消息。但无论这条消息是否真的被记录下来，每次都会运行这个开销很大的String.format 操作。

　　可以像上面代码那样优化性能，即确保只在 log 级别等于某一数值时，才把消息被写入在 log。

　　Java 8中，可以使用 lambda 表达式来提高性能，而不是使用 if。由于使用到 lambda，除非消息真的被记录下来，String.format 不会执行。这应该是Java 8推荐的方法。

　　涉及到性能有很多要考虑的事情……

　　正如我在第一篇中列出代码审查需要留意的事情，本文强调了其中的一些关注点，可能是你们团队在审查时会持续检查的。这取决于你们项目的性能需求。

　　虽然本文是面向所有的开发人员，但大部分例子都是使用Java或 JVM。我最后介绍一些简单的事情，是在审查 Java代码时需要留意的，这样可以让JVM 而不是你自己去优化代码：

• 编写小的方法和类。
• 保持逻辑简单 —— 不要使用深入嵌套的 if 或 循环。

　　代码越容易让人阅读，JIT 编译器越有可能理解你的代码从而去优化它 。 如果代码看上去容易理解和整洁，它的性能也可能很好 —— 这应该很容易在代码审查时发现。

　　总结

　　谈到性能，你要理解通过代码审查，某些方面可以快速得到改善（比如，没有必要的数据库调用），有些方面也很诱人（就像代码级优化）但很可能不会给你们的系统带来很大的改善。