コードレビューするときの視点

普段、Ruby(RailsSinatraといったWAFにおける)のコードレビューするときに、 こういった事を念頭においてレビューする、というものが自分の中だけで閉じている気がしているので、 文章にしてリスト化しておくことでコードレビュー時に、自分自身も漏れが少なくなり、相手にもレビュー箇所を伝えやすいんじゃないかと思案してみた。 静的解析(Rubyであればrubocopやhoundなどを使う)して指摘できる部分は極力はぶいたものとなっています。

TODO: パッと思いついたことで網羅できている感じはないので、増やしていく。

アプリケーションが解決するドメインの視点

ドメインについてレビューすることが一番重要であると考えているので時間をかけるところだったりします。 つまり、問題を解決する戦略についてレビューするところ。

  • 解決しようとする問題が明確になっているか
    • けっこうあったりする。たとえば、p-rにいろいろ含まれていたりするのがその傾向にあって、レビュイーの人が問題を明確になってなかったり
      • やんわり、イシュー分割しましょうと促す
  • 解決する問題に対して粒度が適切かどうか
    • めっちゃ頑張ってオーバースペック過ぎたり、逆にもうちょっと汎用的に作っておいたほうが良かったり

プログラミングという視点

プログラミング言語特有のお作法だったりするものがあると思うのだけど、この3つは必ず見ます。

  • 条件文が明確かどうか
    • if文やcase文の条件文が正しいかどうか
  • データ構造が適切かどうか
  • 各名前付け(クラス名、モジュール名、メソッド名、変数名)が見通しがよいか、妥当かどうか

Ruby固有の視点

  • Enumerableなオブジェクトに対する操作が冗長になっていないかどうか
    • たとえば、Arrayの要素を足し合わせる時に、

      sum = 0 array.each { |v| sum += v }

      と書くより、

      array.inject(:+)

      と書いたほうが見通しがよくなる(と思っているのはinjectおじさんだからかも)

    • 冗長かな、と思ったらEnumerableのメソッドをひと通り確認する

Rails(やSinatra)、各gem固有の視点

モデル(ARを使う場合)

  • N+1問題が起きていないかどうか
    • includes を使うことでだいたいにおいて回避可能
  • LEFT OUTER JOINが起きていないかどうか
    • 基本的にLEFT OUTER JOINを発行するくらいならクエリを分けたほうがロジックの見通しがよくなることが多い(NULLを含めるのは避けたい)
      • その結果N+1問題が発生するのは本末転倒なので、その場合、in句でしっかり1クエリで取れるようにする
  • scopeの使い方は妥当かどうか
    • scope定義してみたはいいが1箇所しか使わない、という場合が多いことがよく見受けられる。意味のある名前にすることでロジックの見通しがよくなるなら構わないが、where句をその場で書いたほうが見通しは良くなることが往々にしてある。
  • >= のような演算の場合に、べたにSQLを書かないで、arel_table を使っているかどうか

コントローラ

  • たとえば、before_action 等で params に値を突っ込んでないか(個人的にジャグリング問題と名づけている)
  • トランザクション境界は明確になっているか

ビュー

  • ロジックを書きすぎていないか
    • helperに分離する

テスト(RSpec + FactoryGirl + RSpec mocksを使う場合)

  • letとlet!は使い分けているかどうか
  • mockとstubは使い分けているかどうか
  • FactoryGirlでテストデータを定義するときにPK(特に、サロゲートキーの場合)を固定の値にしていないかどうか

gem

  • メンテナンスされていないgemを採用してないかどうか
    • 採用を考えている時点で最後のコミットが1年前くらいだとプロダクトに使うものとしては警鐘を出す
      • どうしてもそのgemを使わないと、問題を解決するのに遠回りになったりする場合、そのgemのメンテナになる意気込みで採用する

Golangでエレガントだと思うこと

@kana さんとハッカソンしていて、Golangのどこが好きか? と聞かれた時にうまく説明できなかったのでまとめておきます。 よく、Golangはgoroutineとchannelが取り上げられることが多いと思いますが、 それよりも、僕がGolangGolangたらしめていると考えているものとして、TypeとInterfaceの機構です。 Golangの思想は、他の言語ではこうなんだけど? ということを踏まえてFAQで簡単に説明されています。

http://golang.org/doc/faq#types

例えばFAQのTypesの章にあるサンプルコードで示されているように、

package main

import "fmt"

type Fooer interface {
    Foo() string
    ImplementsFooer()
}

type Bar struct {}
func (b *Bar) Foo() string { return "bar" }
func (b *Bar) ImplementsFooer() { fmt.Println("implements bar") }

type Buzz struct {}
func (b *Buzz) Foo() string { return "buzz" }

func foo(o Fooer) {
    o.Foo()
}

func main() {
    bar := &Bar{}
    fmt.Println(bar.Foo())
    bar.ImplementsFooer()
    foo(bar)

    buzz := &Buzz{}
    fmt.Println(buzz.Foo())
    foo(buzz) // <- コンパイルエラー
}

Barは、関数Foo()ImplementsFooer()を実装しているのでBarFooerを充足している、という表現になります。 Interfaceはよく、「Interfaceを実装する」という表現で使われます。 そのため、言語の文法レベルだと予約語を使ったり何かしら型やクラスに対して宣言するものが多いと思います。 Golangの場合だとInterfaceで定義されている関数を実装しているだけで、そのInterfaceとして振る舞えるので、 「Interfaceを充足している」という表現が適切だと思っています(FAQではsatisfyという英単語が使われています)。

サンプルコードに戻ってみると、関数foo()は、引数に型Fooerを要求しています。 つまり、Fooerを充足しているものであればなんでも渡せるわけです。 逆に、Fooerを充足していなければそこでコンパイルエラーとなります。 型BuzzFoo()は実装していますが、ImplementsFooer()を実装していないことをすぐに検出できます。 コンパイル時に漏れがないことがわかるので安全です。

よくあるDuck Typingのサンプルコード(Wikipediaよりhttp://en.wikipedia.org/wiki/Duck_typing#Implementations)を書いてみると:

package main

import "fmt"

type Duckable interface {
    Quack()
    Feathers()
}

type Duck struct{}
func (d *Duck) Quack()    { fmt.Println("Quaaaaaack !") }
func (d *Duck) Feathers() { fmt.Println("The duck has white and gray feathers.") }

type Person struct{}
func (p *Person) Quack()    { fmt.Println("The person imitates a duck.") }
func (p *Person) Feathers() { fmt.Println("The person takes a feather from the ground and shows it.") }

func InTheForest(duck Duckable) {
    duck.Quack()
    duck.Feathers()
}

func main() {
    donald := &Duck{}
    john := &Person{}
    InTheForest(donald)
    InTheForest(john)
}

静的型付けにおけるDuck Typinpの一設計をエレガントな実装に落とし込めていると思ったので、僕のなかでGolang熱が高まっているわけですね。

octopusに待望のsharding + replication構成がサポートされそう

Railsで、master/slave構成を作る場合や垂直分割する場合などは、octopusに頼る場合が多いです。 ただ、sharding + replicationの構成はずっとpendingのままで、僕も何度かトライしたことがあるのですがうまい設計を練ることができずにそのまま指を加えたまま時が過ぎていました。 が、最近octopusにsharding + replication構成をサポートするPRがありました。

https://github.com/tchandy/octopus/pull/239

ちょっと追いかけてどうなっていくのか注目です。

追記

大きく改変もなくマージされました。これでoctopusでsharding + replication構成を簡単に構築できます。

JSONを扱うときにkeyとfieldを対応づける

Goの、encoding/jsonUnmarshal で、例えばJSONデータのkeyが小文字の場合、自動でcapitalizeしてくれる。

pakcage main

import(
  "encoding/json"
)

type A struct {
  A_Id int
  Field string
}

func main() {
  jsonBlob := []byte(`
    {"a_id": 100, "field": "field"}
  `)
  var a A
  json.Unmarshal(jsonBlob, &a)
  fmt.Printf("%+v\n", a)
}

ここで、JSONの要素にsnake_caseなa_idがkeyになっている。 Goが変数や、構造体のフィールド名はCamelCaseを推奨しているので、CamelCaseなフィールド名にしたい。

type A struct {
  AId int
  Field string
}

Goのドキュメントを読んでいると、

type A struct {
  AId int `json:"a_id"`
  Field string `json:"field"`
}

と書いておくと、UnmarshalしたときにKeyに紐づくフィールドを認識してくれることを知る。 文法として、Goのstructはフィールドにtag(型はstring)を付与でき、この情報はreflectで取得できる

func main() {
  a := A{}
  t := reflect.TypeOf(a)
  field := t.Field(0)
  fmt.Println(field.Tag.Get("json")) //=> a_id
}

便利。

effective-goではない何か

去年からGolangを書き散らかしてみて、だいぶセンス感じとれるようになってきたので、Golangを書くときのイディオムのようなものをまとめてみる。

Golangをコーディングするディレクトリ

簡単な、10行20行くらいで済むコードであればどこで書いてもいいのだけれども、 ライブラリだったりモジュールに分けるレベルの規模のコードだと、コーディングするディレクトリは、$GOPATH直下、または$GOPATHへのsymlinkにしたほうが良い。

例えば、ライブラリを作成しようとしてホスティング場所はGitHubだとする。 その作成しているライブラリを使ってサンプルコードを書こうとすると、 import宣言は、

import (
  "github.com/yoppi/a-library"
)

となる。 ただ、$GOPATHにないとビルドできないので、まだGitHubにpushしていない場合はもちろん、「少し修正して書き直す」という繰り返しの中でGitHubにpushしてgo getしないと手元でコード書けない、という状況は辛い。 そこで、書いているライブラリは、$GOPATH直下 -- たとえば$GOPATHが $HOME/.go/ だとすれば、$HOME/.go/src/github.com/yoppi/a-library -- に作成すると、問題なくビルドできる。 ちなみに、$GOPATHに直接作成するか、$GOPATHへsymlink張るかの流派で分かれている、模様。 僕は、symlink派。

ライブラリ兼実行コマンドありのディレクトリ構成

ライブラリとしても使うのだけど、実行コマンド(たとえばa-execという名前だとする)もあるライブラリを作るときは、

a-library
  `
  + a-exec
  |   `
  |   + main.go
  + a-library.go
  ...

と、実行コマンド名をディレクトリ名にして、直下にmain.goとするディレクトリ構成だとわかりやすいと思っている。 こうしておくことで、go get github.com/yoppi/a-library/a-exec で追加でインストールも可能になる。

ちなみに、実行コマンドだけのものを作るときでそこそこ規模が大きくなる場合、

a-exec
  `
  + bin
  |   `
  |   + a-exec.go
  + module.go
  ...

みたいな構成にしている人が多そう。

構造体を生成するコンストラクタ

構造体を生成するコンストラクタ関数名には、型名に接頭語Newを付けることにしている。 むしろ、それ以外の接頭語が付いている名前だと、他の人が書いたものを読んだ時に戸惑ってしまう。 Golangで組み込み関数であるnew()もあることですし。 そして、この関数は型宣言した、直下に宣言しておくとさらに読む人が把握しやすいので便利。

type A struct {
  // ... member
}

func NewA() *A {
  a := &A{}
  // ...
  return a
}

型T

Golangで、任意の型を表現するのはinterface{}。reflectを頻繁に使って問題を解決しようとするプログラミングだと、intarface{}型を駆使することになる。 これを毎回毎回タイプしてたらFPの消費量激しくなって、辛くなってくるので、aliasとして型Tを宣言しておくと便利だと思う。

type T interface {}

init関数でコマンドライン引数をパースする

Golangでは、init()関数は特別扱いされる。 使い道としては、Javaでいうところのstatic initializerに相当するもの。 たとえば、ライブラリ内コレクションを貯める、みたいなインタフェースがあったとして

package some-library

var hogeCollection map[string]Hoge

func init() {
  hogeCollection = map[string]Hoge{}
}

のように書いておくと安全に初期化できる。 これを利用してコマンドライン引数の初期化などを書いておくとコードがすっきりしそう。 コマンドライン引数の扱いは、flagパッケージを駆使すると便利にパースできる。

import (
  "flag"
)

var (
  a int
  b string
)

func init() {
  flag.IntVar(&a, "a", 0, "args a")
  flag.StringVar(&b, "b", "", "args b")
  flag.Parse()
}

func main() {
  println(a)
  println(b)
}

グローバルに変数を宣言することになるけど、mainに書くよりかは読みやすいかなぁと。

ISUCON3 - 予選から本戦を終えて

TwitterでISUCONに参加したいとぼやいていたら、@f440 さんが一緒にどうかと誘ってくれたので、ISUCON3に参加してきました。

オシャレ怪盗スワロウテイルとして参加し、本戦2位という結果で幕を閉じました。 参加した動機として、自分がWebエンジニアとしてどれくらい通用するのかという腕試しでもあり、また優勝を狙っていたので、あとからじわじわ悔しさが溢れだしながらこのまとめを書いています。

僕も考えていた全体的な心構えとか、方針とかは相方の @f440 さんのエントリ ISUCON3 の参加記録 に読みやすくまとめらているので、合わせて読んでもらえると僕達がどう考えてISUCON3に取り組んだのかが深まると思います。

予選前

今回がISUCON初参加、ということもあり勝手があまりわかっていない状態だったので、@f440 さんにざっと解説してもらい過去の問題(ISUCON2KAYACさんの社内ISUCONRubyに書き直して)をチューニングする、みたいなことをしていました。

予選でやったこと

予選の問題は、メモスニペットアプリ(投稿するテキストがMarkdownで編集できる)でした。 セッションを伴うWebアプリは出題されそうだよね(KAYACさんの社内ISUCONがまさに)という話は事前にしていて、出題されたときに予想どおりでテンションが上がったのを覚えています。 また、公開されたamiからAWSに自前で用意して挑戦するというのも、AWSに慣れた身としては気持ちも楽でした。

やったこととしてそんなに多くなく、

  • クエリのチューニング
  • DBのチューニング
  • Varnishによるページキャッシュ
  • テーブルのインメモリ化

という感じでした。 クエリのチューニング、DBのチューニングは早々に潰せ、 さらに、Varnishをページキャッシュとして導入したことにより一気にスコアを上げることができたまではよかった。 のだけれども、アプリが軽くなってくると、Markdownで変換しているところが重たくなってくる、のは目に見えてわかってきていたのに、どうでもよいチューニングを繰り返して、結局1位を取れず、2日目2位、総合5位で予選通過という結果になりました。

予選が終わってからの反省会で、「なんで巨大なテキスト取ってきてsplitしてるんだよ」「普通Redcarpet使うよね」「カウントしてるところとかRedisでincrするでよいのでは」と、冷静になると効果的なチューニングができることに気づいて少し凹んだりもしました。

予選を終えて

予選の結果があまりにも不甲斐ない終わり方だったので、 メンバーとチャットで予選の反省会しつつ、本戦に向けての準備を各自する、ということを続けていました。

  • 「今までずっと、コンテンツはテキストベースだったよね」
  • Flickrみたいな画像のアップローダーとか来そう?」
  • 「INSERTばかりでなくて、UPDATEやDELETEが大量にあるのでは」
  • SNS系だよなぁ。タイムラインは複雑になると破綻するし」
  • Twitterもどきはそろそろ出題されそう」
  • 「本戦でも多くてもテーブルは3つか4つくらいだよね」

などなどお互い予想しつつ、僕はもっぱらこういったアプリのデータ構造をRDBで持つアプリにしてチューニングする、ということを繰り返していました。 たとえばRDBじゃなくてKVSにする場合はどういう持ち方にするだろうか、キャッシュとの親和性を高めるにはどうすればいいか、等を考えていました。 具体的には、MySQLからRedisへデータ構造に落としこんだり、キャッシュとアプリの親和性を高めるために、ワーカーでアプリの断片を生成しつつ、nginxのSSIで組み立てるみたいなことも特訓していました。

また、KAYACさんが予選で使用したGo製のベンチマークツールも公開してくれていたので、手を加えて予選の問題を改造して、削除や更新も追加して負荷をかけたりして、ベンチマーク作る側からすればどういう問題が出そうか、みたいなことも考えたりしました。

どうでもいいですが、Go、大変良いですね。最近Rubyより書いています。

本戦でやったこと

本戦のお題は、公開レベルを設定できる画像のみ投稿可能なTwitterでした

構成がシングルページアプリケーションと今となっては珍しくないですが、しっかりした作りでチーム内で「シンプルに作られてて良い」みたいな話もしつつ、兎に角プロファイルです。 予選のときに、いろいろ手当たり次第な感じにチューニングを進めてしまったことを反省して、本戦では、アプリを読み込む、プロファイルをとることに注力しようとう話を事前にしていました。

まずは、コードを読みつつ画面を見ながらアプリを操作しつつ静的解析してからデータ構造を把握したところで、 素の状態で、以下のプロファイルを取ります。

  • Apacheのアクセスログ
  • MySQLのクエリログ(long_query_tme=0)
  • Sinatraのアクションログ(enable :logging)

これらのログから、「画像配信」がボトルネックだということが、すぐにわかりました。 DBにはほぼ負荷がないことに若干戸惑いもしました(mysqldumpslowすると一撃でわかります)。

そこで、画像配信をどうすれば速くできるか。午前中はそればかりを話し合っていました。アクションログや、アプリのコードを読みつつ、

  • 初期配置された画像はすべて事前にすべてのサイズで変換しておく
  • アプリで変換済みのものがあれば変換しない
  • 変換したら、保存して2度変換しない
  • アイコンはすべてpublicだよね? これはすべて変換してしまって静的ファイルとして返す
  • 画像も公開範囲は途中で変更できないので、publicなものはアイコンと同様に静的ファイルとして返す
  • 投稿されたときに非同期でワーカーで画像を変換する

という一連の案を素早く閃けたのは、なかなか良い感じでした。 あとは、作業分担して淡々とチューニングをすすめるだけなのですが、

  • 画像のバックアップ先がMacBook Airだった
  • そのままMacBookAirで、画像変換スクリプトを走らせてしまった(MacBook Proあるのに!)
  • ワーカーを作成したけど、思ったほど効果が出ない
    • 最初Watchrで書いたのですが思う通り動かず、Guardにしたのですが、ファイル検知が遅すぎて使い物になりませんでした。あとから、フロントからResqueなりSidekiqに投げればよかったとか思いつき悶絶したり
  • 構成として、1台のみアプリを動かして、4台にnginxを配置してフロントにしたのだけど全台をフロントにしてそれぞれでアプリを稼働させればスケールしてた

と、予選と同じくやはり本番中ではなぜか気づけず、タイムアップ後やチャット上で冷静になった頭で考えるとあれこれアイデアが浮かんでくるというなんともやり切れなかった感が残りました。 ただ、アグレッシブな方法を取らず、基本を忠実に、Failせずに完走できたのは良かったと思います。

まとめ

とにかくISUCONでは、

  • 熱くなっても冷静になる
  • ふつうのWeb技術力
  • アプリケーションを短時間で把握する力
  • Web技術の引き出しを増やして、問題解決のために使えるものを選択できる力
    • 知ってるとかちょっと使えるだけではだめで、これをこう使うと問題をこう解決できる、というレベルにシフトしておく

が、大切だなーと実感しました。 仕事と同じで、問題解決にはまずどこが問題なのかを把握するところから始めます。 ISUCONでは、それを7時間という短い時間内に的確に見極めて、手を入れていくのはとても面白いものがありました。

調度良い難易度の問題を作成してくださったKAYACさん、また、インフラを提供してくださった、データホテルさん、イベントを開催していただいたLINEさん、本当にありがとうございました。 オシャレ怪盗スワロウテイルはここで解散となりますが、また来年リベンジしたいと思っています。