会社のBlogにも書いたのですが、ISUCON 8 予選で負けてきました。
さて、 ISUCON の初期実装の定番として、初心者が書いたようなSQLやコードになっている点が挙げられます。
今回の Go の初期実装もその定番にもれず、初心者がやりがちな、Goの良さを殺してしまうコードがありました。
今回負けた反省点の一つとして、アプリの書き換えを二人でやっていたのでコンフリクトを恐れてそのリファクタリングを怠ったというのもあります。
Goで本戦に参加されるチームの方にはぜひこれを克服してもらいたいと思います。
例: https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L404-L492
e.GET("/api/users/:id", func(c echo.Context) error {
var user User
if err := db.QueryRow("SELECT id, nickname FROM users WHERE id = ?", c.Param("id")).Scan(&user.ID, &user.Nickname); err != nil {
return err
}
loginUser, err := getLoginUser(c)
if err != nil {
return err
}
if user.ID != loginUser.ID {
return resError(c, "forbidden", 403)
}
... [以下数十行]
Sinatra 風のコードを移植しようとするとありがちなのですが、関数名が main.func8 とかになってしまって、Goの良さであるスタックトレースやプロファイルの使い勝手を大きく損ねます。Goでこういう無名関数の使い方はやめましょう。 (Ruby や JavaScript では良いのかという話もありますが、それはまた文化が違うので…)
例えば sort.Slice() にわたす比較関数のようにごく短く、中からDBアクセスなどの処理をしない関数は無名関数にしてもいいです。
また、１つの通常関数の中に無名関数が１つかせいぜい２つあるくらいなら、やはり害は少ないです。
スタックトレースとか flamegraph に main.func6 とか main.func8 とかがバラバラでるのはダメです。
無名関数全体を移動し、適当に名前をつけましょう。
ISUCON であれば事前練習で、なにか機械的な命名規則を決めてしまうと命名に時間を取られずに済みます。
例えば上の "/api/users/:id" を扱う関数であれば、 handle_GetApiUsersId とかで良いです。Goの慣習の命名規則（アンダースコア使わない、 API とか ID とかは Api, Id にしない）からは大きくハズレますが、 ISUCON では悩まず機械的に作業できる利点の方が大きいです。
アプリの書き換えを複数人でやる場合、このリファクタリングは書き換え範囲が大きくてコンフリクトが厳しいので、真っ先にえいっとやってしまうと良いと思います。
アンチパターン: for rows.Next() ループ内でのクエリ実行
例: https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L236-L252
rows, err := db.Query("SELECT * FROM sheets ORDER BY `rank`, num")
if err != nil {
return nil, err
}
defer rows.Close()
for rows.Next() {
...
err := db.QueryRow("SELECT * FROM reservations WHERE event_id = ? AND sheet_id = ? AND canceled_at IS NULL GROUP BY event_id, sheet_id HAVING reserved_at = MIN(reserved_at)", event.ID, sheet.ID).Scan(&reservation.ID, &reservation.EventID, &reservation.SheetID, &reservation.UserID, &reservation.ReservedAt, &reservation.CanceledAt)
こんな感じで、 rows.Next() ループの中でクエリを実行したり、クエリの実行を含む関数を呼び出すと、複数のDBのコネクションを利用してしまいます。
そしてもっと悪いことに、他の prefork 型のアプリではプロセス数を適当に絞ることができるのに対して、GoでDBのコネクションプールを絞ろうとすると上のようなコードでデッドロックの原因になります。デッドロックを避けるためにコネクションプールの上限を設定しないと、たとえCPUが２コアしかないようなMySQLサーバーに対して数百コネクションから並列でクエリを投げてしまい、遅いクエリがどれか解らないとか、MySQL側がデッドロックを誤検出するとか、いろんなトラブルの原因になります。
for rows.Next() ループの中ではそのクエリの結果のフェッチだけを行い、その結果の各行に対する処理は改めて別のループに書きましょう。
アンチパターン: 長い関数での defer rows.Close()
https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L404-L492
上のアンチパターンの亜種ですが、せっかく for rows.Next() ループ内からDBにアクセスする処理を排除しても、 defer rows.Close() を使っていると、そのクエリに使われたコネクションは関数が終わるまでコネクションプールに返却されません。
とはいえ、安易に defer rows.Close() を for rows.Next() ループの後ろに rows.Close() の形で移動するのもダメです。 for rows.Next() ループ内に return が無いか確認しましょう。
上のような問題の「まっとうな」リファクタリング方法は、１つのクエリを実行して結果をフェッチするまでを個別に関数に切り出すことです。そうすると defer rows.Close() が適切なタイミングで実行されます。
しかし ISUCON だといちいちそういったリファクタリングをしている余裕がないかもしれません。そこで sqlx を覚えておくと良いでしょう。
sqlx は、 database/sql の上位互換になっています。 sql.Open を sqlx.Open に書き換えるだけで、そのままのコードが動きます。
そして、 db.QueryRow(...).Scan(&data) を db.Get(&data, ...) に、 db.Query(...), for rows.Next() { var row Record; rows.Scan(&row); records = append(records, row) } のようなパターンを db.Select(&records, ...) に書き換える事ができます。
特に後者が強力で、 rows.Next() と rows.Close() を排除することができるので、コネクションを無駄に大量消費する問題を楽に解決することができます。
それ以外にも便利機能がいくつかあるので、 database/sql を直接使った初心者コードをリファクタリングするときの強力な武器として練習しておくことをおすすめします。
