clean up GIL usuage by zhaojuanmao · Pull Request #32748 · pytorch/pytorch

zhaojuanmao · 2020-01-29T00:33:39Z

Stack from ghstack:

clean up GIL usuage #32748 clean up GIL usuage

This is to follow up PR #30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so

pyRef::to_here() and pyRef::local_value() added GIL
pyRef::pickle and pyRef::unpickle() added GIL release tag
in request_callback_impl, also added GIL as needed
for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function

Differential Revision: D19612337

This is to follow up PR #30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function Differential Revision: [D19612337](https://our.internmc.facebook.com/intern/diff/D19612337/) [ghstack-poisoned]

This is to follow up PR #30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function Differential Revision: [D19612337](https://our.internmc.facebook.com/intern/diff/D19612337/) ghstack-source-id: 97355818 Pull Request resolved: #32748

kostmo · 2020-01-29T00:48:58Z

💊 CircleCI build failures summary and remediations

As of commit 6855011:

1/2 broken upstream at merge base c729614 since Jan 29
Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch origin viable/strict
git rebase --onto viable/strict $(git merge-base origin/master HEAD)
```
If your commit is older than viable/strict:
```
git fetch origin viable/strict
git rebase viable/strict
```
Check out the recency history of this "viable master" tracking branch.
1/2 failures introduced in this PR

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakage:

pytorch_macos_10_13_py3_test (1/1)

Step: "Test" (full log | pattern match details)

Jan 29 01:01:12 RuntimeError: test_autograd failed!

Jan 29 01:01:12 Ran 1046 tests in 307.708s 
Jan 29 01:01:12  
Jan 29 01:01:12 FAILED (errors=1, skipped=13, expected failures=1) 
Jan 29 01:01:12  
Jan 29 01:01:12 Generating XML reports... 
Jan 29 01:01:12 Traceback (most recent call last): 
Jan 29 01:01:12   File "test/run_test.py", line 457, in <module> 
Jan 29 01:01:12     main() 
Jan 29 01:01:12   File "test/run_test.py", line 450, in main 
Jan 29 01:01:12     raise RuntimeError(message) 
Jan 29 01:01:12 RuntimeError: test_autograd failed! 
Jan 29 01:01:12 + cleanup 
Jan 29 01:01:12 + retcode=1 
Jan 29 01:01:12 + set +x

🚧 1 upstream failure recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:

pytorch_windows_build from Jan 29 until Jan 29

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 6 times.

mrshenli

LGTM! Thanks!

I just have some minor questions inline.

Regarding the test failure, looks like test_quantized_rnn is cleared on master. Could you please try rebase to the current master to trigger tests again?

mrshenli · 2020-01-29T03:24:29Z

torch/csrc/distributed/rpc/py_rref.cpp

+  {
+    // acquiring GIL as torch::jit::toPyObject creates new py::object without
+    // grabbing the GIL.
+    pybind11::gil_scoped_acquire ag;


(can be done in followup PR) do we need to use PROFILE_GIL_SCOPED_ACQUIRE to profile this?

mrshenli · 2020-01-29T03:27:50Z

torch/csrc/distributed/rpc/python_rpc_handler.cpp

  pySerialize_ = py::none();
  pyHandleException_ = py::none();
  jitCompilationUnit_ = nullptr;
+  typeParser_ = nullptr;


Curious, it does not seem ScriptTypeParser depends on Python. Is setting it to nullptr here just being defensive or will it indeed hit error in certain destruction order?

just be defensive, do you think whether there is an issue to do so?

mrshenli · 2020-01-29T03:30:18Z

torch/csrc/distributed/rpc/init.cpp

+                    // __setstate__
+                    return PyRRef::unpickle(t);
+                  }),
+              py::call_guard<py::gil_scoped_release>())


would I be correct if I assume this GIL acquirement is moved to rref_impl.cpp?

I added this tag, mainly because PyRRef::unpickle() called pythonrpchandler related stuff that requires GIL. but yes, we moved GIL to rref_impl.cpp as well

Not important, but to be accurate.
In PyRRef::unpickle(), it seems PythonRpcHandler::getInstance().parseTypeFromStr(rfd.type_str_); does not require GIL.

yes, but it could wait for pythonrpchandler to be constructed in another thread, and that thread is trying to aquire GIL, if we do not release GIL in unpickle, it could result in deadlock

torch/csrc/distributed/rpc/request_callback_impl.cpp

xush6528 · 2020-01-29T04:32:56Z

torch/csrc/distributed/rpc/py_rref.cpp

-    if (rref_->isPyObj()) {
-      // UserRRef<py::object>::toHere() calls python_rpc_handler which acquires
-      // GIL.
-      return jit::toPyObject(


2 facts that help me understand jit::toPyObject() (which returns a py::object) requires GIL.

Constructing and destructing an py::object C++ object changes the Python object's ref count.

Copy constructor; always increases the reference count.

Destructor; automatically calls handle::dec_ref().

Ref: https://pybind11.readthedocs.io/en/stable/reference.html#_CPPv4N6object6objectERK6object

Changing Python object's ref count requires acquiring GIL.

The Python interpreter is not fully thread-safe. In order to support multi-threaded Python programs, there’s a global lock, called the global interpreter lock or GIL, that must be held by the current thread before it can safely access Python objects.

Without the lock, even the simplest operations could cause problems in a multi-threaded program: for example, when two threads simultaneously increment the reference count of the same object, the reference count could end up being incremented only once instead of twice.

Ref: https://docs.python.org/3/c-api/init.html#thread-state-and-the-global-interpreter-lock

Combing them together, when constructing or using a py:object C++ object, the GIL should be acquired.

mrshenli

Given @xush6528's comment, we need to extend GIL to cover all occurrences of py::object?

xush6528 · 2020-01-29T04:57:15Z

torch/csrc/distributed/rpc/rref_impl.cpp


 py::tuple RRefForkData::toPyTuple() const {
+  // add GIL as it is contructing a py::object
+  pybind11::gil_scoped_acquire ag;


Do we need GIL for RRefForkData::fromPyTuple as well, since RRefForkData::fromPyTuple is using the py::object subtype, py::tuple? or it has already acquired GIL, how?

sounds good, let me add GIL for fromPyTuple as well

it's a const reference of the py::tuple and does not seem to create/delete py::object. Is it still necessary to use GIL in fromPyTuple.

I guess GIL here can help avoid underneath python object pointed by pyTuple could be mutated?

@mrshenli Accessing (both read and write) a Python object also requires GIL, otherwise, other thread could get GIL and invalidate that Python object.

zhaojuanmao · 2020-01-29T08:14:42Z

@mrshenli I think we added GIL for all py::objects after adding GIL to fromPyTuple

This is to follow up PR #30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function Differential Revision: [D19612337](https://our.internmc.facebook.com/intern/diff/D19612337/) [ghstack-poisoned]

Pull Request resolved: #32748 This is to follow up PR #30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function ghstack-source-id: 97373011 Differential Revision: [D19612337](https://our.internmc.facebook.com/intern/diff/D19612337/)

xush6528

It's good to go now.

mrshenli · 2020-01-29T19:13:55Z

torch/csrc/distributed/rpc/rref_impl.cpp


 py::tuple RRefForkData::toPyTuple() const {
+  // add GIL as it is contructing a py::object
+  pybind11::gil_scoped_acquire ag;


it's a const reference of the py::tuple and does not seem to create/delete py::object. Is it still necessary to use GIL in fromPyTuple.

Summary: Pull Request resolved: pytorch#32748 This is to follow up PR pytorch#30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function ghstack-source-id: 97373011 Test Plan: unit test Differential Revision: D19612337 fbshipit-source-id: 4d09f9b52ba626545ae7d31fea6b671301ed3890

facebook-github-bot · 2020-01-30T07:15:11Z

This pull request has been merged in b5d8982.

facebook-github-bot · 2020-01-30T07:15:26Z

This pull request has been merged in b5d8982.

Summary: Pull Request resolved: pytorch#32748 This is to follow up PR pytorch#30630, we need to have GIL when calling jit::toPyObject(), for some binded functions need to be taged with GIL release if underneath C++ codes requires GIL. so 1. pyRef::to_here() and pyRef::local_value() added GIL 2. pyRef::pickle and pyRef::unpickle() added GIL release tag 3. in request_callback_impl, also added GIL as needed 4. for typeParser, use cached jitCompilationUnit_, also clean it up in cleanUp() function ghstack-source-id: 97373011 Test Plan: unit test Differential Revision: D19612337 fbshipit-source-id: 4d09f9b52ba626545ae7d31fea6b671301ed3890

zhaojuanmao requested review from mrshenli and pritamdamania87 as code owners January 29, 2020 00:33

zhaojuanmao requested a review from xush6528 January 29, 2020 00:36

zhaojuanmao assigned wanchaol and unassigned wanchaol Jan 29, 2020

zhaojuanmao requested a review from wanchaol January 29, 2020 00:37

mrshenli approved these changes Jan 29, 2020

View reviewed changes

xush6528 reviewed Jan 29, 2020

View reviewed changes

mrshenli requested changes Jan 29, 2020

View reviewed changes

xush6528 reviewed Jan 29, 2020

View reviewed changes

xush6528 approved these changes Jan 29, 2020

View reviewed changes

mrshenli approved these changes Jan 29, 2020

View reviewed changes

facebook-github-bot closed this in b5d8982 Jan 29, 2020

facebook-github-bot added the merged label Jan 30, 2020

facebook-github-bot deleted the gh/zhaojuanmao/30/head branch February 2, 2020 15:18

mruberry added the Merged label Oct 28, 2020

Conversation

zhaojuanmao commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostmo commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

Detailed failure analysis

🕵️ 1 new failure recognized by patterns

pytorch_macos_10_13_py3_test (1/1)

🚧 1 upstream failure recognized by patterns:

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xush6528 Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xush6528 Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaojuanmao commented Jan 29, 2020

Uh oh!

xush6528 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 30, 2020

Uh oh!

facebook-github-bot commented Jan 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zhaojuanmao commented Jan 29, 2020 •

edited

Loading

kostmo commented Jan 29, 2020 •

edited

Loading

xush6528 Jan 29, 2020 •

edited

Loading

xush6528 Jan 29, 2020 •

edited

Loading

xush6528 Jan 29, 2020 •

edited

Loading