fresh digitable

めんどくさかったなってことを振り返ったり振り返らなかったりするための記録

github actions でやっているcommit checkの処理を早く終わらせたかった

Android gradle

3行で

  • 平行に処理してみたりキャッシュが効くようにしたが早くはならなかった
  • やってみたかったのでやった
  • 後悔はしていない

その時のPR： github.com

チェックを並列に実行するが効果なし（むしろ増）

これまでのコミットチェックではAndroidのLintとktlint、UnitTestをstepで順番に行っていた。高速化と言ったら並列化だろ、ってことで、PRのタイトルにもした通りまずはこれらのstepを分割してjobに昇格し、平行に実行させてみた。ついでに、共通で行うコンパイルの処理を前段に、レポートの処理を後段で行うようにしたらちょっとしたビルドパイプラインになった。

github actionsでは、jobsの下に列挙したjobはすべて平行に実行される。指定したjobの終了を待ってから開始したいjobはその設定でjobs.needs: <<前段のjob-id>> を書いておく。前段のjob-idは[ ]で囲んで複数指定できる。今回のパイプラインはjobだけ書くと次のようになる。

- jobs
  - compile
  - lint
    - needs: compile
  - ktlint
    - needs: compile
  - unittest
    - needs: compile
  - report
    - needs: [lint, ktlint, unittest]

あとはそれぞれのjobでGradleのキャッシュをリストアする処理などが行われるように設定していく。キャッシュのリストアのオーバーヘッドはあるだろうけどまあちょっとぐらいは早くなるでしょという期待を胸にワークフローを走らせてみたところ、これまで15分程度だった実行時間が20分ぐらいかかるようになってしまった。どうやらキャッシュのリストアが長すぎるというのと、ビルドキャッシュが効いていないというのが原因のようだったのでこれの対策を行うことにした。

キャッシュをちゃんとやる

キャッシュは2GB程度あり、リストアには2分程度かかっていた。これを前段でも中段でもやっているのでどうにかして節約したい。キャッシュにはactions/cacheを使っていて、当初はマニュアルのGradleの例に書いてる設定をほぼそのまま流用していた。しかし、この設定だとキーが変わったりキャッシュを意識的に消す処理をしない限りこれまで使っていたバージョンのwrapperやらcachesのなんやかやが全てキャッシュに残ったままになってしまい *1 、雪だるま式にサイズが大きくなってしまうのではないかと考えた。そこで、キャッシュサイズを小さくしつつ、かつサイズが必要以上に大きくなりすぎないようにするための方法を考えることにした。

現時点でのGradle 6.8.3において、私の環境では次のディレクトリにキャッシュが保存される様子。

  • ~/.gradle
    • caches
    • wrapper
  • <<プロジェクトルート>>/.gradle

このうち、wrapperディレクトリの下にはgradle wrapperのバイナリがバージョンごとに分かれて入っている。これがバージョンごとに200~300MB程度あり、実際に使うのは1つだけなのでそれ以外は不要である。まずはここを削るために次のようにした。

      - name: cache gradle wrapper
        uses: actions/cache@v2
        with:
          path: ~/.gradle/wrapper
          key: ${{ runner.os }}-gradle-wrapper-${{ hashFiles('gradle/wrapper/gradle-wrapper.properties') }}

使用するgradle wrapperのバージョンはgradle-wrapper.propertiesに書いてあるものを使うのでこのファイルのハッシュをキーに使うことにして、このキーでキャッシュが見つからない時は代わりのキャッシュをリストアしないようrestore-keysを指定しないようにした。

あとはライブラリのキャッシュやビルドキャッシュを保存する部分だが、ここはあっさりと、

      - name: cache gradle
        uses: actions/cache@v2
        with:
          path: |
            ~/.gradle/caches
            ./.gradle
          key: ${{ runner.os }}-gradle-cache-${{ github.sha }}
          restore-keys: |
            ${{ runner.os }}-gradle-cache-
      - name: cache build
        uses: actions/cache@v2
        with:
          path: ./**/build
          key: ${{ runner.os }}-assemble-${{ github.sha }}
          restore-keys: |
            ${{ runner.os }}-assemble-

という感じにしてみた。キーにはgithub.shaを付けてcommit checkごとに新しくなるようにした。ただし、~/.gradle/cachesや./.gradleの下にはバージョン番号のディレクトリがあり、Gradleのバージョンが変わるごとに増えていくと考えられるがこれを消す処理は入れていない。消そうとせずにここのキーにもgradle-wrapper.propertiesのハッシュをつけてもいいかもしれない。

この改善によって、2GB近くあったキャッシュが500MB程度になり、20分に膨れ上がった実行時間が17分にまで抑えられた。キャッシュのない初回の実行なので次回以降はもう少し短くなるはず。あとはmasterブランチにマージするワークフローでも同じようなこと*2をやってやれば次のPRの時にもこのキャッシュを引き継げる。

ふりかえり

  • 平行化した結果トータルで見ると実行時間が増えている（ターンアラウンドタイムも微増）ので今の規模なら素直にstepで直列に実行した方がよかったかもしれない。
  • 平行に実行したいタスクを今後増やしていけるようになったということにして個人的には納得した。
  • タスクではなくモジュールで分けてみてもよかったかもしれない。
  • github actionsはstepの設定を共通化できたらいいのにと思う。どんどんコピペしなきゃいけないんだけどyamlだからインデントの深さをうっかり間違えると怒られるので。

*1:自分のPCの~/.gradleの下を見てそう思った

*2:テストだけのシンプルなものでよい

テストの後片付けでLiveData.removeObserver()すると怒られることがある

Kotlin Android testing

はじめに

そんなことをやる必要はありません。あくまでもネタとしてお読みください。

今回の：

org.jetbrains.kotlinx:kotlinx-coroutines-core:1.4.3
org.jetbrains.kotlinx:kotlinx-coroutines-test:1.4.3
androidx.lifecycle:lifecycle-livedata-ktx:2.2.0

顛末

ViewModelのテストで、LiveDataやFlowの監視を開始したり終了したり流れてきた値をチェックする処理を簡単にできるようにしたいと考えて、そのようなクラスを作り現行の処理と置き換えたところ、次のようにコルーチンの処理が全部終わってないといって怒られた。

kotlinx.coroutines.test.UncompletedCoroutinesError: Unfinished coroutines during teardown. Ensure all coroutines are completed or cancelled by your test.
    at kotlinx.coroutines.test.TestCoroutineDispatcher.cleanupTestCoroutines(TestCoroutineDispatcher.kt:178)
        ...

試しにコルーチンを起動させている個所をコメントアウトするなどしてみたが状況は変わらず、特に変わったことしてないはずなのにおかしいなーなどと思いながらコードを見直したところ、良かれと思って後片付けの箇所に足した処理があったのを思い出した。

override fun finished(description: Description?) {  // TestWatcher の関数
    super.finished(description)
    observers.forEach { (l, o -> l.removeObserver(o) }   // これ
    scope.cancel()
}

該当の処理をコメントアウトしたところ、怒られが解消した。

解説

androidx.lifecycle.liveData()やandroidx.lifecycle.asLiveData()で作ったLiveData *1 は、activeになった時に内部のコルーチンスコープを使ってFlow.collect()を開始する。その後、inactiveな状態になると一定時間（デフォルトでは5秒）待ったあとでFlow.collect()のJobをキャンセルするようになっている。この一定時間待つという処理にdelay()が使われている。ということは当然コルーチンが起動しているので、これが完了するのを待たなければならない。テスト冒頭でDispatchers.setMain()をしつつ、kotlinx.coroutines.test.runBlockingTest()やそれに相当する関数を使うなどして、例えば、

    runBlockingTest {
        observers.forEach { (l, o -> l.removeObserver(o) }
    }

としてやることでdelay()を含んだJobを消化できる。

ちなみに

androidx.lifecycle.asFlow()の中ではJobのキャンセル時にGlobalScopeなコルーチンが起動するのでこれも終わるまで待てないとだめ（一敗）。

*1:asLiveData()は内部でliveData()を呼んでいる

fluxとかMVIみたいな構造のアプリを作ってみたかった　その5

Android Kotlin

前回： akihito104.hatenablog.com

前回からの進捗としては、NavigationDelegateをActivity, Fragmentに持たせることにしたり機能追加をやっていた。その中でViewModel以下のクラスをリファクタリングしたところしっくりくるような形になったのでまとめてみる。

クラスの構成について

Androidのデータバインディングのスタイルをなるべく守りたいので、ViewModelをDataBindingに渡してデータのプロパティやイベントリスナの関数とバインドしたいという大前提がある。また、RepositoryのインタフェースはFlowを返すかsuspend関数にして値を返すことにしているので、FlowからLiveDataに変換する必要がある *1 。そのためにはCoroutineContextが必要なので、ViewModel.viewModelScope.coroutineContextを使うことにする。そのような前提と、これまでの実装を踏まえつつクラスごとの役割分担を見直した結果、次のような構成になった。

この構成の基本的なアイディアは、ViewModelをイベントリスナの部分と状態遷移の部分とに分割して、それぞれの処理を別のクラスに委譲するということ。ViewModelの主な役割は状態のFlowをLiveDataに変換するのみになる。また、ViewModelはviewModelScopeを持っていたり、LiveDataを持たせる都合上、Androidの世界とビジネスモデルとの境界に立って両者の橋渡しを行う役割も暗に持つことになった。イベントリスナや状態を表すためのデータはViewModel側が別途定義する。

イベントリスナはActionsクラスに委譲する。当初、このクラスにはMVIのIntentのような役割を持たせたいと考えて、イベントバスに流れてくるイベントを捕まえて個別のFlowを作ることだけをやっていたのだが、いまいち役割が軽く見えていて不要かもしれないと思い始めていた。しかし、今回の見直しで、Actionsにイベントリスナを実装させることでUIイベントの生成から個別のFlowに流すまでを担当することになり、よりMVIのIntentに近づいたように思う。また、こうすることでイベントを流す具体的な処理や具体的なイベントクラスを隠蔽することができるようになった。

状態遷移はViewModelSourceクラスに委譲する。このクラスはもともとViewStatesという名前で、Viewの状態遷移を担当していたのでやること自体はあまり変わらない。ただ、このクラスにActionsを注入する都合から、このクラスにもイベントリスナを実装させActionsに委譲する形にしておけば、ViewModelはこのクラスだけに依存すればよいことになる。もしそうなったとき、ViewStatesとViewModelとの違いは状態をFlowで流すかLiveDataで持つかだけになるので、実質的にViewStatesはほぼViewModelと同じものになるんだなと考えて名前をViewModelの方に寄せる（ViewModelSource）ことにした。

状態遷移の処理もこれを機に見直した。当初はViewのプロパティをそれぞれ個別のFlowやLiveDataにわけて更新していたが、複数のFlowやLiveDataが関係しあうとコードが複雑になってよくなかったので、ViewModelから提供される状態データのインタフェースを実装したデータクラスを使って一元的に管理することにした。このあたりのことはまた別の記事に書こうと思うが、ざっくりいうとRxやFlowにあるscan()のような仕組みを使って、イベントが流れてくるごとに新しい状態のデータを生成して流すというような感じの実装になっている。

流れてきたイベントや状態遷移の結果、画面遷移したりSnackbarで表示するようなちょっとしたメッセージフィードバックをしたいときはそれ用のFlowを用意してActivityやFramgent向けにイベントを流す。

ここまでやってきて

当初はfluxやMVIのようなものを目指して作ってきたが、自分の理解力が足りなかったり実装力が無かったりしてバグが多く、ちょっと足したり削ったり移したりするとすぐ壊れてしまって難しかった。しかし、今の形に至り、ある程度わかりやすくまとまったのではないかと感じている。今、これは何かと尋ねられたら、大きくなってしまったViewModelを分割するための一つの方法、と答えると思う。今の実装をもっと洗練させていくとfluxやMVIといったものに近づいていくのかもしれないが、もうあまり意識していない。ただ、ある程度の達成感は得られたものの、この試みがここで終わるのかと聞かれるとそれも違う気がしていて、同じタイトルでこのまま続けていくかもしれない。

次回: akihito104.hatenablog.com

*1:いずれFlowのままDataBindingに渡せるようになるだろうから必要なくなるかも