Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: Initial implementation #497

Merged
merged 34 commits into from
Sep 15, 2021
Merged

rust: Initial implementation #497

merged 34 commits into from
Sep 15, 2021

Conversation

eagletmt
Copy link
Member

@eagletmt eagletmt commented Sep 1, 2021

参考実装を Rust に移植します。

備考

enum 的な値の扱い

https://scrapbox.io/ISUCON11/%E7%A7%BB%E6%A4%8D%E3%81%AB%E9%96%A2%E3%81%97%E3%81%A6%E6%B0%97%E3%81%AB%E3%81%AA%E3%81%A3%E3%81%A6%E3%81%84%E3%82%8B%E7%82%B9 で挙げていたように、Rust では UserType、CourseType、DayOfWeek、CourseStatus については enum で定義することにしています。

middleware の定義

利用している actix というフレームワークでの middleware 定義がどうしても長くなってしまうので src/middleware.rs というファイルに分割しています。

f64 同士の比較

https://scrapbox.io/ISUCON11/%E7%A7%BB%E6%A4%8D%E3%81%AB%E9%96%A2%E3%81%97%E3%81%A6%E6%B0%97%E3%81%AB%E3%81%AA%E3%81%A3%E3%81%A6%E3%81%84%E3%82%8B%E7%82%B9 で挙げていましたが、現時点の Go 実装では == での比較になっています。
これのせいで clippy (Rust の標準の linter) もデフォルトで warn ではなく error になって通っていません。
https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp

% cargo clippy
    Checking isucholar v0.1.0 (/home/eagletmt/.clg/github.com/isucon/isucon11-final/webapp/rust)
error: strict comparison of `f32` or `f64`
  --> src/util.rs:42:12
   |
42 |         if arr[0] != v {
   |            ^^^^^^^^^^^ help: consider comparing them within some margin of error: `(arr[0] - v).abs() > error_margin`
   |
   = note: `#[deny(clippy::float_cmp)]` on by default
   = note: `f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp

error: aborting due to previous error

error: could not compile `isucholar`

To learn more, run the command again with --verbose.

#470 でわざわざ消してるようなので、このまま変わらないようであれば #[allow(clippy::float_cmp)] をつけて clippy をつけて黙らせることにします。

@eagletmt eagletmt changed the title [WIP] rust: Initial implementation rust: Initial implementation Sep 5, 2021
Copy link
Member Author

@eagletmt eagletmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP 外しました。description に書いた点以外で golang 版から意図的に変えて実装している箇所にコメントつけました

let files = ["1_schema.sql", "2_init.sql"];
for file in files {
let data = tokio::fs::read_to_string(format!("{}{}", SQL_DIRECTORY, file)).await?;
let mut stream = pool.execute_many(data.as_str());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute_many() で複数の文からなる SQL を実行できるので golang 版の GetDB() は移植しておらず、したがって db.go に相当するファイルも用意してません

let (user_id, _, _) = get_user_info(session)?;

let mut tx = pool.begin().await.map_err(SqlxError)?;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

↓に相当するチェックがここに入ってないんですが、CourseType や DayOfWeek を enum で定義しているので期待していない文字列が入っていた場合は AddCourseRequest のデシリアライズに失敗して 400 を返す挙動になっています。ただこの Rust 実装の場合はエラーメッセージが golang 版とは異なるものになってしまうので、ここの 400 のエラーメッセージも検証するようであれば別の実装にする必要があります。現状はベンチマーク通ってますが。

if req.Type != LiberalArts && req.Type != MajorSubjects {
return echo.NewHTTPError(http.StatusBadRequest, "Invalid course type.")
}
if !contains(daysOfWeek, req.DayOfWeek) {
return echo.NewHTTPError(http.StatusBadRequest, "Invalid day of week.")
}

.fetch(pool.as_ref());
let mut totals = Vec::new();
while let Some(row) = rows.next().await {
let total_score: sqlx::types::Decimal = row.map_err(SqlxError)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IFNULL(SUM(`submissions`.`score`), 0) のところが MySQL の decimal 型で返ってくるようで i64 や f64 に直接デシリアライズできなかったのでこういう実装にしてます。
GPAの統計値のところのクエリも同様。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

へー (クエリで as integer とかしてもそうなのかな)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

クエリ側を変えれば i64、f64 で受け取れますね。MySQL の CAST() を初めて使いましたが↓のような変更でも prepare も load も通ってます。ただカラムに別名をつけるみたいなデータに影響の無い変更というわけではないので、もし CAST() するなら全初期実装に入れる必要があるかなと思ってます。

diff --git a/webapp/rust/Cargo.lock b/webapp/rust/Cargo.lock
index b87a0dc..9631200 100644
--- a/webapp/rust/Cargo.lock
+++ b/webapp/rust/Cargo.lock
@@ -333,12 +333,6 @@ dependencies = [
  "memchr",
 ]
 
-[[package]]
-name = "arrayvec"
-version = "0.5.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "23b62fc65de8e4e7f52534fb52b0f3ed04746ae267519eef2a83941e8085068b"
-
 [[package]]
 name = "askama_escape"
 version = "0.10.1"
@@ -983,7 +977,6 @@ dependencies = [
  "listenfd",
  "log",
  "mime",
- "num-traits",
  "serde",
  "serde_urlencoded",
  "sqlx",
@@ -1611,17 +1604,6 @@ dependencies = [
  "zeroize",
 ]
 
-[[package]]
-name = "rust_decimal"
-version = "1.15.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c5446d1cf2dfe2d6367c8b27f2082bdf011e60e76fa1fcd140047f535156d6e7"
-dependencies = [
- "arrayvec",
- "num-traits",
- "serde",
-]
-
 [[package]]
 name = "rustc_version"
 version = "0.2.3"
@@ -1888,7 +1870,6 @@ dependencies = [
  "percent-encoding",
  "rand 0.8.4",
  "rsa",
- "rust_decimal",
  "rustls",
  "sha-1",
  "sha2",
diff --git a/webapp/rust/Cargo.toml b/webapp/rust/Cargo.toml
index ca71faf..39d07a6 100644
--- a/webapp/rust/Cargo.toml
+++ b/webapp/rust/Cargo.toml
@@ -18,9 +18,8 @@ lazy_static = "1"
 listenfd = "0.3"
 log = "0.4"
 mime = "0.3"
-num-traits = "0.2"
 serde = { version = "1", features = ["derive"] }
 serde_urlencoded = "0.7"
-sqlx = { version = "0.5", default-features = false, features = ["macros", "runtime-actix-rustls", "mysql", "chrono", "decimal"] }
+sqlx = { version = "0.5", default-features = false, features = ["macros", "runtime-actix-rustls", "mysql", "chrono"] }
 tokio = { version = "1", features = ["fs", "process"] }
 ulid = "0.4"
diff --git a/webapp/rust/src/main.rs b/webapp/rust/src/main.rs
index 84fc8f2..6a66b5e 100644
--- a/webapp/rust/src/main.rs
+++ b/webapp/rust/src/main.rs
@@ -2,7 +2,6 @@ use actix_web::web;
 use actix_web::HttpResponse;
 use futures::StreamExt as _;
 use futures::TryStreamExt as _;
-use num_traits::ToPrimitive as _;
 use sqlx::Arguments as _;
 use sqlx::Executor as _;
 use tokio::io::AsyncWriteExt as _;
@@ -792,8 +791,8 @@ async fn get_grades(
         }
 
         // この科目を受講している学生のtotal_score一覧を取得
-        let mut rows = sqlx::query_scalar(concat!(
-            "SELECT IFNULL(SUM(`submissions`.`score`), 0) AS `total_score`",
+        let totals = sqlx::query_scalar(concat!(
+            "SELECT CAST(IFNULL(SUM(`submissions`.`score`), 0) AS SIGNED INTEGER) AS `total_score`",
             " FROM `users`",
             " JOIN `registrations` ON `users`.`id` = `registrations`.`user_id`",
             " JOIN `courses` ON `registrations`.`course_id` = `courses`.`id`",
@@ -803,12 +802,9 @@ async fn get_grades(
             " GROUP BY `users`.`id`",
         ))
         .bind(&course.id)
-        .fetch(&mut tx);
-        let mut totals = Vec::new();
-        while let Some(row) = rows.next().await {
-            let total_score: sqlx::types::Decimal = row.map_err(SqlxError)?;
-            totals.push(total_score.to_i64().unwrap());
-        }
+        .fetch_all(&mut tx)
+        .await
+        .map_err(SqlxError)?;
 
         course_results.push(CourseResult {
             name: course.name,
@@ -833,35 +829,29 @@ async fn get_grades(
 
     // GPAの統計値
     // 一つでも修了した科目(履修した & ステータスがclosedである)がある学生のGPA一覧
-    let gpas = {
-        let mut rows = sqlx::query_scalar(concat!(
-            "SELECT IFNULL(SUM(`submissions`.`score` * `courses`.`credit`), 0) / 100 / `credits`.`credits` AS `gpa`",
-            " FROM `users`",
-            " JOIN (",
-            "     SELECT `users`.`id` AS `user_id`, SUM(`courses`.`credit`) AS `credits`",
-            "     FROM `users`",
-            "     JOIN `registrations` ON `users`.`id` = `registrations`.`user_id`",
-            "     JOIN `courses` ON `registrations`.`course_id` = `courses`.`id` AND `courses`.`status` = ?",
-            "     GROUP BY `users`.`id`",
-            " ) AS `credits` ON `credits`.`user_id` = `users`.`id`",
-            " JOIN `registrations` ON `users`.`id` = `registrations`.`user_id`",
-            " JOIN `courses` ON `registrations`.`course_id` = `courses`.`id` AND `courses`.`status` = ?",
-            " LEFT JOIN `classes` ON `courses`.`id` = `classes`.`course_id`",
-            " LEFT JOIN `submissions` ON `users`.`id` = `submissions`.`user_id` AND `submissions`.`class_id` = `classes`.`id`",
-            " WHERE `users`.`type` = ?",
-            " GROUP BY `users`.`id`",
-        ))
-        .bind(CourseStatus::Closed)
-        .bind(CourseStatus::Closed)
-        .bind(UserType::Student)
-        .fetch(&mut tx);
-        let mut gpas = Vec::new();
-        while let Some(row) = rows.next().await {
-            let gpa: sqlx::types::Decimal = row.map_err(SqlxError)?;
-            gpas.push(gpa.to_f64().unwrap());
-        }
-        gpas
-    };
+    let gpas = sqlx::query_scalar(concat!(
+        "SELECT CAST(IFNULL(SUM(`submissions`.`score` * `courses`.`credit`), 0) / 100 / `credits`.`credits` AS DOUBLE) AS `gpa`",
+        " FROM `users`",
+        " JOIN (",
+        "     SELECT `users`.`id` AS `user_id`, SUM(`courses`.`credit`) AS `credits`",
+        "     FROM `users`",
+        "     JOIN `registrations` ON `users`.`id` = `registrations`.`user_id`",
+        "     JOIN `courses` ON `registrations`.`course_id` = `courses`.`id` AND `courses`.`status` = ?",
+        "     GROUP BY `users`.`id`",
+        " ) AS `credits` ON `credits`.`user_id` = `users`.`id`",
+        " JOIN `registrations` ON `users`.`id` = `registrations`.`user_id`",
+        " JOIN `courses` ON `registrations`.`course_id` = `courses`.`id` AND `courses`.`status` = ?",
+        " LEFT JOIN `classes` ON `courses`.`id` = `classes`.`course_id`",
+        " LEFT JOIN `submissions` ON `users`.`id` = `submissions`.`user_id` AND `submissions`.`class_id` = `classes`.`id`",
+        " WHERE `users`.`type` = ?",
+        " GROUP BY `users`.`id`",
+    ))
+    .bind(CourseStatus::Closed)
+    .bind(CourseStatus::Closed)
+    .bind(UserType::Student)
+    .fetch_all(&mut tx)
+    .await
+    .map_err(SqlxError)?;
 
     tx.commit().await.map_err(SqlxError)?;

@@ -0,0 +1,100 @@
// ----- int -----

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

標準ライブラリに sum/max/min があるので、全体的に for ループではなくそっちを使った実装にしています。
ただし Rust において f64 は PartialOrd ではあるが Ord ではないため max/min は使えず、reduce で実装してます。
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.max

command: clippy
args: --manifest-path webapp/rust/Cargo.toml

# TODO: Run benchmarker with -no-load
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

golang 版に対して prepare を実行する CI が追加されたらそれを真似して Rust 版にも足すようにします #534

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!

@eagletmt
Copy link
Member Author

eagletmt commented Sep 6, 2021

今日気付いたけど sqlx 内でスレッドが panic してるね…… qualify での参考実装も同様に panic してた。sqlx 側の issue はこのへん。
launchbadge/sqlx#1078
launchbadge/sqlx#1358
再現条件はトランザクションが commit されずに drop されてロールバックが走るとき。この panic が発生してもレスポンスは正常に返せていて動作には影響しない。
スレッドが panic するのでパフォーマンスへの影響はもしかしたら多少あったかも…… とはいえ再現するのはエラーを返すときのみなので、prepare ではほぼ確実に発生するけど load 中はほとんど発生してなかったと思う。

issue にあるように drop される前に明示的に rollback すれば回避できるのでとりあえずそのワークアラウンドを入れる。できるだけ今のコードを変えずに回避したいけど難しいかなぁ。

@eagletmt
Copy link
Member Author

eagletmt commented Sep 6, 2021

sqlx の不具合を完全に理解したので fetch_one() と fetch_optional() を使わないようにすることでスレッドの panic を回避できた

webapp/rust/src/main.rs Outdated Show resolved Hide resolved
webapp/rust/src/main.rs Show resolved Hide resolved
.fetch(pool.as_ref());
let mut totals = Vec::new();
while let Some(row) = rows.next().await {
let total_score: sqlx::types::Decimal = row.map_err(SqlxError)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

へー (クエリで as integer とかしてもそうなのかな)

.bind(&req.description)
.execute(&mut tx)
.await;
if let Err(sqlx::error::Error::Database(ref db_error)) = result {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

原作の動作ではここでロールバックしてる。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ナイスキャッチ。直しました ec5d09c

.bind(&req.message)
.execute(&mut tx)
.await;
if let Err(sqlx::error::Error::Database(ref db_error)) = result {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@eagletmt
Copy link
Member Author

portal-dev で動かしてみていて気付いたんですが Go の exec.Command(...).Run() は stdin、stdout、stderr を /dev/null に向けてたので一応揃えました。
679e929
cp、rm、mkdir は正常時は元々出力が無いので変わりませんが、zip は出力があるのでそれを出さないように。

@eagletmt
Copy link
Member Author

sqlx の panic を直すパッチをたぶん作れた launchbadge/sqlx#1439
↑を使った場合の初期実装 https://github.com/isucon/isucon11-final/tree/rust/patched-sqlx 。ベンチマークは panic 無しで完走できている。

この PR で入れてるワークアラウンドと比べて fetch_one()/fetch_optional() が2行以上の結果が返ってきたときに余計なデシリアライズが走らないという差がありそうだけど、現実的に fetch_one()/fetch_optional() にそういうクエリは渡さないし僕が書いたこのパッチが正しい修正なのかもまだよく分からないので、この PR のワークアラウンドのままのほうが安牌かなと思う。

@sorah
Copy link
Member

sorah commented Sep 15, 2021

この PR のワークアラウンドのままのほうが安牌かなと思う。

db.rs のコメントで言及しとくくらいはやってもいいかも。おまかせ。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants