代碼審查應該關注什麼：性能

前言

猛然發現，這個系列的文章很適合既懂前端，又會 java 的人來看看。對，說的就是那些全棧，因為文中提到的都是用 java 代碼來做示例。今日早讀文章由 @ 唐先僧翻譯分享。

正文從這開始~

在本系列代碼審查文章的第三篇，我們準備討論在代碼審查中性能方面需要關注哪些事情。

和所有的架構/設計一樣，一個系統非功能性的性能需求也應該優先考慮。不管你是在開發必須在幾個毫微秒內響應的低等待時間交易系統、還是在創建一個需要儘快響應用戶的購物網站，或者是在開發一款管理 「待辦事項」的移動應用，你都應該有「太慢了」 的概念。

我們一起來看看審查者在代碼審查在中應該關注的影響性能的事情。

首先

這個功能有硬性的性能需求嗎？

被審查的代碼屬於有公開的 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 中使用反射要比不使用反射慢。如果你正審查的代碼中使用了反射，問一下是否真的需要。

超時

當你在審查代碼時，你也許不知道一個操作合理的超時是多少，但是你應該考慮「當超時在倒計時時，對系統的其他方面會議什麼影響？」。作為審查者你應該考慮最壞情況-在5分鐘的超時時間內系統會阻塞掉嗎？如果這個時間設置的是1秒最壞會發生什麼情況？如果代碼的作者不能為說明為什麼選定超時的長度，作為審查者你也不知道選定的超時長度的優缺點，這個時候需要邀請知道這個超時影響的人來討論。不要等待用戶來投訴性能方面的問題。

並行

代碼使用了多個線程來完成一個簡單任務？使用多線程是添加了時間和複雜性而不是提高性能？在最新的 Java 中，這可能比直接創建一個線程更隱蔽：代碼中使用了 Java 8最新的並發 stream 但是並沒有享受到並發的好處？例如：使用並發 stream 處理少量的數據，或者使用 stream 處理一個很簡單的任務，有可能比使用串列 stream 處理更慢。

在上面的例子中，新增加的 parallel 調用好像沒有給我嗎帶來任何好處--這裡 stream 作用於一條 Tweet， 也就是最多140個字元的字元串。將並發應用到這麼短的字元上可能不會帶來任何性能的提高，相反將字元串分割到各個並發線程所付出的代價要比並髮帶來的好處要高。

正確性

這些事情不一定會影響系統的性能，但是由於好多線程環境強相關，所以它們和性能這個主題相關。

代碼中為多線程選擇了正確的數據結構？

在上面的代碼中，在第12行使用了一個 ArrayList來存儲所有的 sessions。但是這段代碼會被多個線程調用，尤其是 onOpen 方法，所以 sessions 應該使用一個線程安全的數據結構。在這個例子中，我們有很多選擇：可以使用 Vector，也可以使用 Collections.synchronizedList() 來創建一個線程安全的 List，但是最好的選擇可能是 CopyOnWriteArrayList， 因為這個 list 被修改的頻率要遠遠低於被讀取的頻率。

代碼是否容易產生競爭條件？

在多線程環境中，非常容易寫出導致競爭條件的代碼。例如：

儘管遞增的代碼只有一行（第16行），很有可能會出現另外一個線程在當前線程讀取並存儲新的 value 之間加1。作為審查者，需要注意是 get 和 set 組合不是原子性的的情況。

正確的使用鎖了嗎？

這一條和競爭條件相關，作為審查者，你應該確認不允許多線程以可能衝突的方式修改值。這樣的代碼可能需要使用 synchronization、locks 或者 atomic variables 來控制對代碼塊的更改。

為代碼添加的性能測試是否有用？

因為很容易寫出很差的測試代碼。或者測試用例中使用的數據和真實數據完全不一樣，這可能會給出誤導我們的結果。

緩存

使用緩存是阻止過多的外部請求的一種方式，同時它自身也會帶來問題。如果你審查的代碼中使用了緩存，你應該關注一些常見問題，比如：緩存項目的有效處理不正確。

代碼級別的優化

如果你是代碼審查者，同時又是開發者，接下來的這一節也許有你喜歡建議的優化方法。作為一個團隊，你需要知道性能對你有多麼重要，以及這些類別的優化對你的代碼是否有用。

對大多數不是構建低延時應用的團隊，這些優化可能劃入提前優化（Premature Optimization）類別。

代碼中是否使用了不必要的同步/鎖？如果代碼總是在單線程上運行，鎖就是不必要的開銷。

代碼中是否使用了不必要的線程安全數據結構？例如：Vector 是否可以用 ArrayList 來替換？

代碼中是否使用常用操作性能很差的數據結構？例如：使用了單向鏈表，但是需要經常在其中搜索一個單項？

代碼使用懶載入是否會更好？

是否將 if 語句或者其他可以短路的邏輯判斷放在最前面？

是否使用了很多字元串格式化？可以更高效的完成嗎？

log語句是否使用了字元串格式化？有提供給 if 語句根據 log 級別判斷是否輸出 ?

上面的代碼只輸出級別為 FINEST 的信息。但是開銷昂貴的字元串格式化每次都會執行，不管消息是否輸出。

像上面的代碼這樣修改可以提高性能，只有需要輸出的消息才格式化。

在 Java 8 中，不使用 if 這樣的模板代碼也可以獲得這些性能。由於使用了 lambda 表達式，字元串格式化操作只有在 log 輸出時才會執行。這應該是 Java 8 中最好的方法。

在考慮性能問題時要擔心的問題非常多。。。

正如在我的第一篇文章中列出的關注列表一樣，本文重點介紹了你的團隊可能在代碼審查中一直檢查的一些領域。這取決於你的項目對性能的要求。

儘管本文是針對所有的開發者，很多例子是特別針對 Java/JVM。我還是想用 Java 代碼審查者幾條簡單的建議來結束本文，這樣可以讓 JVM 來優化你的代碼而不是你自己親自來優化：

方法和類盡量小

邏輯盡量簡單--沒有很深的 if 判斷或者循環

你的代碼對人類的可讀性越高，JIT 編譯器就越有機會足夠理解你的代碼並優化。這在代碼審查中是很容易發現的--如果代碼看起來整潔可讀，也就有很大可能良好運行。

結論

在考慮性能問題時，需要記住：有的問題在代碼審查階段界需要解決（例如：不必要的資料庫訪問），有的問題暫時提交評論就可以（如代碼級別的優化），因為這些事情可能不會給你的系統帶來足夠的價值。

關於本文

譯者：@唐先僧

譯文：http://www.jianshu.com/p/8cf4f3fe5506

作者：@ Trisha Gee

原文：https://blog.jetbrains.com/upsource/2015/08/06/what-to-look-for-in-a-code-review-performance/

喜歡這篇文章嗎？立刻分享出去讓更多人知道吧！