かみぽわーる

kamipo's blog

ISUCON10予選ふりかえり

ISUCON10予選おつかれさまでした。ISUUMOいい問題でしたね。過去出題側を担当したこともある身でも、参加者の完全攻略に対する怖れもあって仕様が肥大化するなか今回これだけコンパクトな仕様のアプリケーションでこれだけ楽しめる出題をしたのマジですごいと思いました。

今回の問題はMySQLかつ検索ヘヴィな問題で僕のバックグラウンドに向いてる問題にも関わらず、ずっと手を動かしていたわりに効果の高い施策に取り組めず、あらためてISUCONの難しさを痛感したしこれぞISUCONなのだなあと思います。

僕の文章読解が遅く仕様理解にとても時間を要するという性質から、これまでのISUCONでは常にアプリケーションの仕様や性質を理解できる前に時間的制約からあらゆる決断を迫られるという状況にあり、この状況で仕様や性質を理解できていたとしたらできた正しい決断をしていくのは本当に難しいと思っていて、今回ずっと手を動かしていたわりに結果が振るわなかったのもひとえにその難しさだなあと個人的には思っています。

時間を節約するために個人的にとても時間を要する仕様書を読むのを疎かにして失敗したこともあるし、DBボトルネックが支配的ではない問題でスロークエリの対処に時間を投下してアプリケーション仕様を最後までちゃんと理解しないまま失敗したこともある。僕の技術的なバックグラウンドであるMySQLボトルネックの支配的な要素ではない近年のISUCONで、チームで唯一普段Rubyを書いている僕が、自分の有限の時間を何に投下するかというのをその場その場で決断していくのはとてもハードだったなと感じた。だけれど、そのことの精度を上げていくことがISUCONでの結果に繋がっていくのではないかというのが今回ISUCON10予選を終えて感じた個人的な総括になります。

今回手元で動かせるアプリケーションでもあったので、今日起きてから予定の合間合間に予選で決断できなかったDB側の感想戦をしてみたけど、いやぁこれがその場でできないのがマジでISUCONだなあとあらためて思った。未来のいつかの自分のために今日のこの気持ちをここにしたためておきます。

DB側感想戦

感想戦: index comment by kamipo · Pull Request #12 · soudai/isucon10-qualify · GitHub

8e98179ad6 以降のスキーマ変更(index削除とか)効いてなかった by kamipo · Pull Request #13 · soudai/isucon10-qualify · GitHub

感想戦: Add descending index `popularity DESC` by kamipo · Pull Request #14 · soudai/isucon10-qualify · GitHub

不要なindex削除 by kamipo · Pull Request #15 · soudai/isucon10-qualify · GitHub

Optimize get '/api/recommended_estate/:id' by kamipo · Pull Request #16 · soudai/isucon10-qualify · GitHub

Optimize post '/api/chair/buy/:id' by kamipo · Pull Request #17 · soudai/isucon10-qualify · GitHub

Avoid N+1 in post '/api/estate/nazotte' by kamipo · Pull Request #18 · soudai/isucon10-qualify · GitHub

Treasure Dataを退職します

急なお知らせですが、8月31日をもってTreasure Dataを退職することになりました。

今後の活動についてはいまのところなにも決まっていないので、自分になにができるのか、どんなニーズがあるのか、いろいろ相談に乗ってもらえるとうれしいです。

きっかけはというと、長年Railsコントリビューター/メンテナーとして並々ならぬ思いで活動してきたんですが。

どのぐらいがんばっていたかというと、たとえば2020年8月時点のコミット数ベースの今年のアクティビティでいうと、上位10人のアクティビティを母数にするとその半数が僕になります。

rails/rails contributors 2020-01-01 - 2020-08-26

Rails 5.0以降のも置いておきます。

rails/rails contributors 2019-01-01 - 2019-12-31
rails/rails contributors 2018-01-01 - 2018-12-31
rails/rails contributors 2017-01-01 - 2017-12-31
rails/rails contributors 2016-01-01 - 2016-12-31

さすがにこんなにがんばっとるねんからこれ仕事ってことにならん?という思いをずっと持っていて、オンライン飲みで友人にそのことを相談したらメンテナーのポジションで選考してもらえて、そこではマッチングには至らなかったんですが、それでなんか吹っ切れたというか。

自分になにができるのか、どんなニーズがあるのか、どんな選択肢があるのか、それを探すのがいま一番やりたいことだなと。

こんな時期なのでなかなか難しいとは思うんですが、いろいろ相談に乗ってもらえたり、あとずっと引きこもってこんな活動を続けていたため知り合いが少ないので、誰かしら繋いでくれたりするとうれしいです。人生の新たな楽しみを見つけたいです。

とりあえず飲みにいきたい。遊びに誘ってくれても全然オッケーです。

Ruby 2.7.0でキーワード引数として渡された引数なのかどうかフラグを確かめる方法

class Hash
  class << self
    def ruby2_keywords_hash?(hash)
      !new(*[hash]).default.equal?(hash)
    end

    def ruby2_keywords_hash(hash)
      _ruby2_keywords_hash(**hash)
    end

    private def _ruby2_keywords_hash(*args)
      args.last
    end
    ruby2_keywords(:_ruby2_keywords_hash) if respond_to?(:ruby2_keywords, true)
  end
end


RUBY_VERSION # => "2.7.0"

def passed_kw?(*args)
  Hash.ruby2_keywords_hash?(args.last)
end
ruby2_keywords(:passed_kw?) if respond_to?(:ruby2_keywords, true)

passed_kw?({ a: 1 }) # => false
passed_kw?(a: 1) # => true

hash = { a: 1 }
passed_kw?(hash) # => false
passed_kw?(**hash) # => true

kw = Hash.ruby2_keywords_hash(hash)
passed_kw?(kw) # => true
passed_kw?(**kw) # => true

説明書こうとしたけどだいぶめんどくさかったのでコードで感じ取ってほしいんですが、Ruby 2.7.0以降Ruby 3に向けてキーワード引数渡しの非互換をハンドリングするためにruby2_keywordsを使ってキーワード引数渡しされたhashオブジェクトにフラグを付けることができる、というかRuby 2.6以前とRuby 2.7以降の両方サポートしたいライブラリメンテナはこのフラグを付けて回らなければRuby 3ではArgumentErrorでお前はもう死んでいる状態になっています。

フラグが付けれるのはいいとしてRuby 2.7.0ではこのフラグが付いてるhashオブジェクトなのかそうじゃないhashオブジェクトなのか確かめる方法が公式には提供されてなくて、どこで呼び出されたときにフラグを付ける必要があって、ちゃんとフラグが付いたままお届け先のメソッドまでたどり着いてるのかのデバッグが死ぬほどめんどくさかった。

これはmameさんのハックを見て知った方法で、ようはフラグが付いてるhashオブジェクトかそうじゃないオブジェクトかで違う振る舞いをする処理を通らせてその結果を観測することでどっちだったかを確かめるという方法です。

このフラグが付いてるhashオブジェクトかそうじゃないオブジェクトかで違う振る舞いをする処理がRubyの世界からはほとんど存在しないので、普段Rubyでコードを書いてる常人が自力では気づけんやろって方法で、一部のメソッド(initializeとかmethod_missingとか)をRubyのコードから直接呼び出すんじゃなくてCのコードから間接的に呼び出されるときにフラグが付いてるhashオブジェクトをdupするので、dupされずにそのままのオブジェクト(object_id)だったらフラグが付いてなかったってことでdupされて別のオブジェクトが観測されたらフラグが付いてたオブジェクトだったという技を使っています。

原理が分かったので既存のメソッドでこの用途に丁度いいメソッドを探した結果、Hash.newにhashのデフォルト値としてフラグ付きかもしれないhashオブジェクトをsplat渡ししてHash#defaultで取り出して観測するという方法が一番シンプルであろうというところに至り、この技を使ってRailsでもキーワード引数完全分離への対応を進めています。

github.com

このフラグ付きかどうか確かめる方法があるのかないのか常人では思い至らん問題にライブラリメンテナは直面すると思いますが、Ruby 2.7.1にはフラグ付きかどうか確かめる方法が公式にバックポートされる予定なので、これで警告が多い日も安心ですね。

github.com

ISUCON9予選ふりかえり

なにもできなかったし書くことないわーって思ってたけど、チームのふりかえりをGitHubのissueに書いてたらふつうにこれ外に書けばいいなってなったのでここに記す。

  • アクセスログから得られる情報の解像度が相対的に低くなってきたと感じた。ざっくりどのエンドポイントが遅いとか何回叩かれてるとかの概観を知るのは依然として重要だけど、近年は遅いエンドポイントの処理はめちゃくちゃ複雑になってきていて、たとえば/buyが遅いぐらいの情報の解像度では情報量が足りてなかった。
  • 外部APIへのリクエストが絡むような問題への心の準備とかできてなかった。なんなら準備してきてないし予選では出ないでほしいなとか思っていた。
  • プログラマとしての筋力が足りてなかった。たとえばアクセスログからの情報では足りないってなったときにときにじゃあやばそうなところ全部にprint文書いて測るわ!ぐらいの気概が必要だった。外部リクエストが遅いとかロック競合で時間がかかっていると遅いけどあまりリソースは食ってないように見えるしそういう面でもprint文書いてでも知りたい情報ログに出したるわぐらいの気概が必要だった。
  • 外部APIへのリクエストが遅いかもってなったときに遅いリソースへのリクエストを多重化するようなプログラミングを普段してなかったのでひよった。きれいに言語のAPIやライブラリが使えなくてもたとえばThread.newしまくってでもやったるわぐらいの気概が必要だった。
  • もはやデータベースに入ってるデータだけじゃなくて外部APIのレスポンスも含めてアプリケーションを構成するデータソースだという考えで挑まなければならないと思った。実際外部APIのレスポンスの内容がリクエストに対して不変な内容かそうでないか、不変ならキャッシュ可能かという考えに至れていなかった(もしキャッシュ可能だと分かれば不慣れなリクエスト多重化プログラミングを避けることができたかもしれない)。
  • 自分たちに相性がいい過去問で練習してできることをやればいけそうって過信してしまった。苦手だった問題も過去にはあるのにそういう苦手分野に向き合うことをおざなりにしてしまったところはいなめなかった。

ISUCON9予選、手も足もでなかったけど、いい問題だと思ったし楽しめた。近年データベースがボトルネックじゃない問題で僕の楽しみがあまりないからISUCONから遠ざかってたところあったけどISUCON8予選がデータベースのロック競合を出題に盛り込んできて、そういう問題ならやってみたかったって思ったし、今回の問題は去年の予選問題のようでもありそこからさらに去年の本選問題を足してひねりが加えられたような一筋縄ではいかない感じであった。今回誘ってくれたid:Soudaiさんと一緒に戦ってくれたid:sugyanにまじ感謝、楽しかったです。

Rails 6.0の複数DBでリードレプリカのテストするのたぶん大変

Rails 6.0の複数DBのレビューしてるときに気づいたことなんですけど、たぶんリードレプリカからデータを読むテストをするのたぶん大変だと思われます。

うちの業務のアプリでActive Recordが更新を検知できない方法でデータが更新されるとテストがコケるという問題が以前にあり、これと同じ構造の問題がマスターのコネクションで更新したときマスターのコネクションのクエリキャッシュはクリアされるけどリードレプリカのコネクションのクエリキャッシュは残ったままというのがあるよね、というのをテストコードで示そうと思ったときのことである。

github.com

通常RailsアプリでDBつかったテストをするとき、テストの中で変更されたデータを毎回初期状態に戻すのにフィクスチャーをロードし直すのは時間がかかって効率がわるいので、テストケースに入る前にトランザクションを開始しといてテストケース終わったらロールバックして変更をなかったことにすることで初期状態に戻し、時間のかかるフィクスチャーロードし直しを避けるということが効率のために行われています。

この、効率のためにテストケース終わったらロールバックして変更をなかったことにするためにテスト内のトランザクションはネストしたトランザクションになっていて実際にコミットは起きないということが複数DBのリードレプリカと相性がとてもわるい。実際にコミットが起きないことにはリードレプリカに更新がレプリケートされてこないのでずっと時が止まった状態なのである。

あたかもマスターで更新が起きたっぽいときにリードレプリカにも更新が伝搬しているかのように見せかけるため、Active Recordはテストのときデフォルトですべてのコネクションプールをマスターのコネクションプールにすり替えるということで対処している。コミットが起きないんだったら全部マスターに接続してテストすればいいじゃないってやつです。しかしこれをされるとマスターではなくリードレプリカからデータを読んでることをテストしたいときにめっちゃこまるという話である。

github.com

コード読んだ限りではフィクスチャー毎回読み直しモードのときは setup_shared_connection_pool は呼ばれないから大丈夫そうに見えるけど、自分が試したときにはフィクスチャー毎回読み直しモードのときでも setup_shared_connection_pool の効果を自力で打ち消さないとまったくリードレプリカに接続できなくてめっちゃ苦労したので、先人の記憶としてここに書き記しておきます。同じ問題にぶち当たったりぶち当たらなかったりなんか分かった人いて教えてくれたらここに追記します。

あと、リードレプリカでクエリキャッシュ残る問題はがんばってテスト書いて修正される運びとなったのでこれで更新の多い日も安心ですね。

github.com

Rails 6.0でDeprecatedになるActive Recordの振る舞い3つ

Deprecatedにした経緯というか背景が伝わってるのかどうかアレだと思ったので、ここに日本語にて書き記しておく。

Active Recordのuniqueness validatorはデフォルトでcase sensitiveな比較をするんですが、これが、文字列のデフォルトのcollationがcase insensitiveMySQLと相性が悪く、DB上のUNIQUE制約と一致しない振る舞いだったりINDEXが効率よく使えずDBが死ぬみたいな問題を引き起こしていました。

例: 本当にあったRailsの怖い話

僕も主に仕事のコードレビューで過去に何度もこの問題を指摘してきたわけですが、去年ぐらいにmakimotoさんにこれRails側でなんとかならないんですか的なこと言われてそりゃ同僚にRailsコミッターおったら仕事で遭遇したRails側のあらゆる問題はRails側でなんとかなってほしいわなって思ったので気合いでなんとかすることにしたという次第です。

いないとは思いますが、MySQLなどでデフォルトのcollationがcase insensitiveな文字列をあえてわざとcase sensitiveな比較をしていた奇特なユーザーの皆様は今後明示的に case_sensitive: true オプションを指定していただくことになります。

where.not(a: ..., b: ...) の結果が where(a: ..., b: ...) の補集合になるようになります。逆にいうと6.1になるまでは where.not(a: ..., b: ...) の結果は where(a: ..., b: ...) の補集合になるとは限りません。

想像してほしいんですが、血液型B型の男性(私です)の集合があったとして、それのNOTを取った集合は血液型B型の男性以外になってほしいと思いませんか?僕は思います。現状、Active Recordの where.not は血液型B型の男性という集合を反転した結果として、男性でもなく血液型B型でもない集合を返します。もしこれが想像していた振る舞いと違う場合、アプリケーションは暗黙のうちにバグってる可能性があるので where.not を使っている皆さんは手元のコードをよく確認してみるのがよいのではないかと思います。

これは説明がむずかしいというか面倒なんですけど、たとえばTopicのツリー型の掲示板みたいなものがあったとして、トップレベルのTopic、かつその中でも子スレッドを持つものをscopeとして定義したいとします。

class Topic < ActiveRecord::Base
  scope :toplevel, -> { where(parent_id: nil) }
  scope :children, -> { where.not(parent_id: nil) }
  scope :has_children, -> { where(id: Topic.children.select(:parent_id)) }
end

# Works as expected.
Topic.toplevel.where(id: Topic.children.select(:parent_id))

# Doesn't work due to leaking `toplevel` to `Topic.children`.
Topic.toplevel.has_children

子スレッドを持つという条件を has_children として抽出して利用したいわけですが、これは現状ほぼ意図した通りには動きません。というのもscopeのメソッドチェインは条件が累積したrelationを伝搬させるのにクラスグローバルな状態を汚染する実装方法を現状とっており、scope定義内はその汚染されたクラスグローバルな状態の影響を受けるためです。

現在、我々がどのようにしてこの問題を回避しているかというと Topic.unscoped.children.select(:parent_id) のように unscoped をつけるのを忘れないように頑張る、というものですが、6.1からは unscoped をつけるのを忘れないように頑張らなくてもよくなるということになります。

逆にいうと、現状scope定義内でモデルクラスから直接scopeやquery methodsを呼び出してるケースではユーザーの期待に反して意図せず別のscopeの状態が注入されている可能性があるので、今一度手元のコードを確認されるのがよいかと思います。

以上、よろしくお願いいたします。

Rubyで安全な文字列リテラルかどうかを判別したい

Rails 5.2からRails SQL Injection ExamplesにあるようなSQLインジェクションを防ぐ仕組みが導入されて、Post.order(params[:order])みたいなコードは心温まる正規表現によるチェックをパスしないと危険とみなされるようになって、お前が安全やと思うんやったらPost.order(Arel.sql(params[:order]))しろってことになった(rails/rails#27947)。

これはRails 5.0のときにparamsがHashのサブクラスじゃなくなったときに比べればマシだけど、明らか安全やと思ってるリテラルRailsに危険とみなされて既存のアプリケーションによったら非常にわずらわしい。たとえばDiscourseというRailsアプリは5.2に上げるときにこれの影響をモロに受けるんやけどっていうお気持ちを表明しています(rails/rails#32995 (comment))。

そこでこのわずらわしさを軽減する方法を考えたんですけど、もし#orderに渡される値がユーザーのリクエスト由来の未知の値でなくソースコード上に直に書かれた文字列リテラルだったとしたら、これは安全ってことにしていいと思いませんか?僕はそう思いました。

ソースコード上に直に書かれた文字列リテラルかどうかを完全に判別できる方法は今のところおそらく存在しないと思うけど、ユーザーのリクエスト由来じゃないかどうかをできるときだけできる範囲で区別するというのだと、# frozen_string_literal: trueのときにユーザーのリクエスト由来の値とちがって文字列リテラルはfrozenになるという性質が利用できそうです。という方法を実装したのが以下。

github.com

これにはひとつ物言いがついて、"#{user_input}"がfrozenにならんのやったらええけどそこがなー的なこと言われてマジカーってなってたら、interpolated stringを判別する方法を教えてくれる神が降臨した(rails/rails#33330 (comment))。

# frozen_string_literal: true

dynamic = DATA.read
constant = "bar"
interpolated = "#{dynamic} DESC"

puts "[dynamic] frozen: #{dynamic.frozen?}, interned: #{dynamic.equal?(-dynamic.dup)}"
puts "[constant] frozen: #{constant.frozen?}, interned: #{constant.equal?(-constant.dup)}"
puts "[interpolated] frozen: #{interpolated.frozen?}, interned: #{interpolated.equal?(-interpolated.dup)}"

__END__
foo
[dynamic] frozen: false, interned: false
[constant] frozen: true, interned: true
[interpolated] frozen: true, interned: false

パッと見なにを判別してるんかよくわからんかったので同僚のRubyコミッターに聞いたらプリントデバッグしながらいろいろ調べてくれた結果すべてを理解することに成功したのでその成果をここでシェアしたいと思います。

これはfrozen literalがRuby 2.5で導入されたfstring tableに登録されているものと同一かどうかを判別していて、constant literalだとfstring tableに登録されたインスタンスを返すけどinterpolated literalだとただのfrozenなだけのインスタンスになるという性質の違いを利用しています。String#-@がfstringを返すのでconstant.equal?(-constant.dup)はfstring tableからlookupしなおしたインスタンスが自分自身と同一だったら自分はfstringだったということでつまりconstant literalだったということがわかります。ちなみに.dupが必要なのはRuby 2.5では自分がfrozenだったらString#-@はfstringではなく自分をそのまま返してしまう問題があるからで、この問題はruby-headでは解決済みなので.dupは要らなくなります(ruby/ruby@256411b)。

この方法は判別しようとするinterpolated literalのバリエーションだけfstring tableに登録してしまうので、なんか他にリーズナブルな方法ないかなって調べてくれたんですけど、今のところなさそう、ユースケース次第ですかねハハハーって感じでお開きとなりました。

以上、Ruby 2.5以上で文字列リテラルを判別する方法でした。