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

#312: add a feature to check MLE #442

Merged
merged 3 commits into from
Jun 16, 2019
Merged

#312: add a feature to check MLE #442

merged 3 commits into from
Jun 16, 2019

Conversation

kmyk
Copy link
Member

@kmyk kmyk commented Jun 12, 2019

close #312

うっかり MLE しないようにメモリ使用量を監視する機能を足します。

具体的な仕様は次のようになります。

  • それぞれのテストケースで:
    • メモリ使用量が 500MB より大きいなら目立たせて出力して警告
    • メモリ使用量が 100MB より大きいならとりあえず出力
    • メモリ使用量がそれ以下なら邪魔なので非表示 (オプションを指定で出力)
  • 最後には常にメモリ使用量を出力 (同じ条件で必要なら目立たせて警告)

実装には GNU time (/usr/bin/time とかにあるやつであって、shell builtin の time コマンドとは異なる) を利用しています。
メモリ使用量を監視するのは一般に面倒で (ある時刻の使用量でなくて実行開始から終了まででの全時刻の最大を取らないとだめ、監視対象のプロセスをきちんと指定しないとだめ、別プロセスのメモリ使用量を好き勝手に取得するのはセキュリティの問題がある)、 他のよい方法は思い付きませんでした。
もちろん time が常に存在するとは限らないですし、存在しても互換性の問題がないとも限らないので、とりあえず使ってみて動くかどうかを判断する処理も含んでいます。

@fukatani これもレビューが簡単ではないものですが、よろしくお願いします 🙇

@kmyk kmyk requested a review from fukatani June 12, 2019 16:05
@@ -142,6 +142,8 @@ def get_parser() -> argparse.ArgumentParser:
subparser.add_argument('-t', '--tle', type=float, help='set the time limit (in second) (default: inf)')
subparser.add_argument('-i', '--print-input', action='store_true', help='print input cases if not AC')
subparser.add_argument('-j', '--jobs', metavar='N', type=int, help='specifies the number of jobs to run simultaneously (default: no parallelization)')
subparser.add_argument('--print-memory', action='store_true', help='print the amount of memory which your program used, even if it is enough small')
Copy link
Contributor

Choose a reason for hiding this comment

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

https://eitopi.com/enough-tukaikata
一応small enoughが正しそうです。

Copy link
Member Author

Choose a reason for hiding this comment

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

ありがとう。修正しました

(ところでこのような1行程度の自明な修正なら suggestion 機能を使うのがおすすめです。提案した側の名前が commit log に乗るという利点があります)

@fukatani
Copy link
Contributor

PRありがとうございます。
ちゃんとテスト書いてますし、よさそうに見えます!

@fukatani
Copy link
Contributor

Windowsなどtimeが入っていない場合になにか警告した方がいいでしょうか?

@fukatani fukatani assigned kmyk and unassigned fukatani Jun 14, 2019
@kmyk
Copy link
Member Author

kmyk commented Jun 14, 2019

time が入ってない場合は簡単に警告が出ます。
すこし不親切ですが、ユーザに問題を理解させたからと言って GNU time を入れることができるものとも思えないので、これで十分かなと思っています。

https://github.com/kmyk/online-judge-tools/blob/30affb2286138c969794a6a57f7b9429e02da5b1/onlinejudge/_implementation/command/test.py#L202

@kmyk
Copy link
Member Author

kmyk commented Jun 14, 2019

CI が落ちてるのは POJ が落ちているためです。無視してマージしてくれてかまいません。

@kmyk kmyk assigned fukatani and unassigned kmyk Jun 15, 2019
@kmyk
Copy link
Member Author

kmyk commented Jun 15, 2019

@fukatani CI 再実行かけました

Copy link
Contributor

@fukatani fukatani left a comment

Choose a reason for hiding this comment

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

LGTM

@fukatani fukatani merged commit 594a473 into master Jun 16, 2019
@fukatani
Copy link
Contributor

@kimiyuki
しまった!すいません、誤ってsquash and mergeしてしまい、このPRでのコミットをひとつにまとめてしまいました。
問題ありましたら、revirtするなりして、再マージするなどの方法があります。

@kmyk
Copy link
Member Author

kmyk commented Jun 17, 2019

できれば squash はしない方がよいですが、この程度ならわざわざ修正するほどでもないと思うのでこのままでいきましょう。

@kmyk kmyk deleted the issue/312 branch June 17, 2019 13:38
@fukatani
Copy link
Contributor

@kmyk
ありがとうございます。
お手数ですが、もしよければ設定を変えてsquashマージを禁止していただけないでしょうか?
私がレビューをしている他のリポジトリではバンバンsquashしているので、またうっかりしかねないなと思って。

test

Setting→Optionsで下の方にスクロールしていくとMergeというメニューがあります。
そこでAllow Merge commitsにのみチェックを入れて他は外せばいいです。

@kmyk
Copy link
Member Author

kmyk commented Jun 17, 2019

@fukatani 設定しました。
どうしてわざわざ squash したのかは不思議に思ったのですが、そういうことだったのですね。これはかなり仕方なさそう。
他にもこの手の慣習の差による面倒があったら教えてください。設定で解決できるものなら設定で解決しますので。

@fukatani
Copy link
Contributor

@kmyk 設定ありがとうございます!なにかあればまた相談します。

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

Successfully merging this pull request may close these issues.

add a feature to check MLE
2 participants