Skip to content

Use GIL to guard decref of jit::toPyObj return value in processRpc#38376

Closed
mrshenli wants to merge 7 commits intogh/mrshenli/175/basefrom
gh/mrshenli/175/head
Closed

Use GIL to guard decref of jit::toPyObj return value in processRpc#38376
mrshenli wants to merge 7 commits intogh/mrshenli/175/basefrom
gh/mrshenli/175/head

Conversation

@mrshenli
Copy link
Copy Markdown
Contributor

@mrshenli mrshenli commented May 13, 2020 •
edited
Loading

Stack from ghstack:

Differential Revision: D21540179

This was referenced May 13, 2020
Closed
Closed
Closed
Closed
@mrshenli mrshenli requested a review from xush6528 May 13, 2020 02:56
mrshenli pushed a commit that referenced this pull request May 13, 2020
Use GIL to guard decref of jit::toPyObj return value in processRpc
f084739 
ghstack-source-id: e76b942
Pull Request resolved: #38376
@xush6528
Copy link
Copy Markdown
Contributor

xush6528 commented May 13, 2020 •
edited
Loading

This PR is for MessageType::PYTHON_RREF_FETCH_CALL.

There are more sites, right?
MessageType::PYTHON_CALL, and MessageType::PYTHON_REMOTE_CALL.

UPDATE:
Checked those 2 sites. They both have a scoped gil acquire enclosing the returned temp py::object.
But they don't have a comment line mentioning this subtlety.

All known sites are resolved?

xush6528
xush6528 reviewed May 13, 2020
View reviewed changes
Comment thread torch/csrc/distributed/rpc/request_callback_impl.cpp Outdated
namespace {

py::object toPyObj(IValue value) {
SerializedPyObj toSerializedPyObj(IValue value) {
Copy link
Copy Markdown
Contributor

@xush6528 xush6528 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's only used by MessageType::PYTHON_RREF_FETCH_CALL.

Can we move under case MessageType::PYTHON_RREF_FETCH_CALL and make it a named lambda, like the auto markComplete = [...](...){...}; lambda on line 116.

Copy link
Copy Markdown
Contributor Author

@mrshenli mrshenli May 13, 2020

Choose a reason for hiding this comment

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

Sure, edited, but the downside is that we will need to capture this lambda in two nested callbacks. I am not sure if the extra overhead worth it here.

Copy link
Copy Markdown
Contributor

@xush6528 xush6528 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mrshenli

Should be fine.

If there is no capture by value, the closure type is typically trivially copyable and trivially moveable.
http://www.cplusplus.com/forum/general/202662/

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 13, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit fbb7f05 (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

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.

See how this bot performed.

This comment has been revised 14 times.

mrshenli pushed a commit that referenced this pull request May 13, 2020
Use GIL to guard decref of jit::toPyObj return value in processRpc
00251dc 
ghstack-source-id: 5cfb289
Pull Request resolved: #38376
@mrshenli mrshenli mentioned this pull request May 13, 2020
Closed
@mrshenli mrshenli mentioned this pull request May 13, 2020
Closed
xush6528
xush6528 reviewed May 13, 2020
View reviewed changes
Comment thread torch/csrc/distributed/rpc/request_callback_impl.cpp Outdated
@mrshenli mrshenli mentioned this pull request May 13, 2020
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented May 14, 2020

@mrshenli merged this pull request in afa4dbd.

@mrshenli mrshenli mentioned this pull request May 16, 2020
Closed
This was referenced May 16, 2020
Closed
Closed
Closed
Closed
@facebook-github-bot facebook-github-bot deleted the gh/mrshenli/175/head branch May 17, 2020 14:18
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@facebook-github-bot
Use GIL to guard decref of jit::toPyObj return value in processRpc (p…
a3536c0 
…ytorch#38376)

Summary: Pull Request resolved: pytorch#38376

Test Plan: Imported from OSS

Differential Revision: D21540179

Pulled By: mrshenli

fbshipit-source-id: 082fa5f11da7fc1f083710b498e72abc5ba2c244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xush6528 xush6528 xush6528 approved these changes

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrshenli @xush6528 @facebook-github-bot @mruberry