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.

Sign up for GitHub

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

Jump to bottom

Fix stack getter segfault caused by is_shutting_down and gil #9681

Merged
merged 4 commits into from
Jan 5, 2023
Merged

Fix stack getter segfault caused by is_shutting_down and gil #9681

merged 4 commits into from
Jan 5, 2023

Conversation

daquexian
Copy link
Contributor

@daquexian daquexian commented Jan 3, 2023 •
edited
Loading

Fixes https://github.com/Oneflow-Inc/OneTeam/issues/1861 ，在 TryCloseDefaultSession 之前设置 is_shutting_down 为 true（#9502 这个 PR 在 TryCloseDefaultSession 里引入了 Sync 操作），并把 python c 对象的指针保存在 PyFrame 对象里，GetCurrentFrame 里不再需要重复获取，修复因为没有拿到 gil 锁导致的 segfault

@daquexian daquexian changed the title set shutting down flag to true as the first step of atexit_hook Fix stack getter segfault caused by is_shutting_down and gil Jan 4, 2023
@daquexian daquexian requested review from oneflow-ci-bot and removed request for oneflow-ci-bot January 4, 2023 10:14
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Speed stats: 
GPU Name: GeForce GTX 1080 









❌ OneFlow resnet50 time: 139.7ms (= 13970.1ms / 100, input_shape=[16, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 165.3ms (= 16525.4ms / 100, input_shape=[16, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.18 (= 165.3ms / 139.7ms)

OneFlow resnet50 time: 84.6ms (= 8458.6ms / 100, input_shape=[8, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 100.5ms (= 10051.7ms / 100, input_shape=[8, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.19 (= 100.5ms / 84.6ms)

OneFlow resnet50 time: 57.2ms (= 11432.1ms / 200, input_shape=[4, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 77.6ms (= 15516.9ms / 200, input_shape=[4, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.36 (= 77.6ms / 57.2ms)

OneFlow resnet50 time: 43.9ms (= 8782.2ms / 200, input_shape=[2, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 83.0ms (= 16591.0ms / 200, input_shape=[2, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.89 (= 83.0ms / 43.9ms)

OneFlow resnet50 time: 40.4ms (= 8088.5ms / 200, input_shape=[1, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 68.4ms (= 13687.0ms / 200, input_shape=[1, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.69 (= 68.4ms / 40.4ms)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

CI failed when running job: cuda-misc. PR label automerge has been removed

@github-actions github-actions bot removed the automerge label Jan 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Speed stats: 
GPU Name: GeForce GTX 1080 









❌ OneFlow resnet50 time: 140.0ms (= 13998.7ms / 100, input_shape=[16, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 159.6ms (= 15956.3ms / 100, input_shape=[16, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.14 (= 159.6ms / 140.0ms)

OneFlow resnet50 time: 84.8ms (= 8483.4ms / 100, input_shape=[8, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 102.1ms (= 10209.4ms / 100, input_shape=[8, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.20 (= 102.1ms / 84.8ms)

OneFlow resnet50 time: 57.7ms (= 11541.5ms / 200, input_shape=[4, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 78.9ms (= 15770.1ms / 200, input_shape=[4, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.37 (= 78.9ms / 57.7ms)

OneFlow resnet50 time: 44.5ms (= 8902.8ms / 200, input_shape=[2, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 72.3ms (= 14467.3ms / 200, input_shape=[2, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.63 (= 72.3ms / 44.5ms)

OneFlow resnet50 time: 40.2ms (= 8039.6ms / 200, input_shape=[1, 3, 224, 224], ddp, world size=2)
PyTorch resnet50 time: 75.6ms (= 15128.1ms / 200, input_shape=[1, 3, 224, 224], ddp, world size=2)
✔️ Relative speed: 1.88 (= 75.6ms / 40.2ms)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/9681/

@liujuncheng liujuncheng merged commit 19bb0da into master Jan 5, 2023
@liujuncheng liujuncheng deleted the fix_shutting_down_flag branch January 5, 2023 02:18
@daquexian daquexian mentioned this pull request Mar 21, 2023
Merged
mergify bot added a commit that referenced this pull request Mar 22, 2023
@daquexian @oneflow-ci-bot @mergify
fix segfault and infinite loop in python stack getter (#9955)
9e87ac4 
1. <del>#4737 这个很早之前的 PR 在
python 退出后会跳过 tensor 的析构，导致 oom，当时这么做的原因已经不记得了，现在来看不跳过也想不到会有什么问题，所以本 PR
恢复为正常执行析构。</del>
    更新：CI 遇到问题，待进一步定位，这个 PR 先不包含这部分改动了。
原 oom 问题可能和 #9681 这个 PR 把设置
is_shutting_down 的时机提前了有关
3. 修复线程不安全导致的 python stack getter 偶发 segfault 的 bug

---------

Signed-off-by: daquexian <[email protected]>
Co-authored-by: oneflow-ci-bot <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liujuncheng liujuncheng liujuncheng approved these changes

@BBuf BBuf BBuf approved these changes

@jackalcooper jackalcooper Awaiting requested review from jackalcooper

@oneflow-ci-bot oneflow-ci-bot Awaiting requested review from oneflow-ci-bot

Assignees
No one assigned
Labels
bug python system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daquexian @liujuncheng @BBuf