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

avoid copying ActorTableData when NodeMananger updates an actor to GCS #5244

Merged
merged 3 commits into from
Jul 26, 2019
Merged

avoid copying ActorTableData when NodeMananger updates an actor to GCS #5244

merged 3 commits into from
Jul 26, 2019

Conversation

micafan
Copy link
Contributor

@micafan micafan commented Jul 22, 2019 •
edited
Loading

What do these changes do?

  1. Avoid copying ActorTableData when NodeMananger updates/registers an actor to GCS
  2. Fix comments

Related issue number

Linter

  • I've run scripts/format.sh to lint the changes in this PR.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15553/
Test PASSed.

jovany-wang
jovany-wang approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

LGTM

src/ray/raylet/node_manager.cc
// TODO(swang): If this is an actor that was reconstructed, and previous
// actor notifications were delayed, then this node may not have an entry for
// the actor in actor_regisry_. Then, the fields for the number of
// reconstructions will be wrong.
if (actor_entry == actor_registry_.end()) {
actor_info_ptr.reset(new ActorTableData());
Copy link
Contributor

Choose a reason for hiding this comment

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

make_shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

No matter to copy a smart pointer here I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/36477022/boost-initializing-shared-pointer-reset-vs-make-shared/36477600

In general, make_shared will be more efficient I think.

raulchen
raulchen approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@raulchen raulchen left a comment

Choose a reason for hiding this comment

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

LGTM. Can you resolve the conflicts? and then we can merge it.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1820/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1821/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15647/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15648/
Test PASSed.

@raulchen raulchen merged commit 6f682db into ray-project:master Jul 26, 2019
@raulchen raulchen deleted the avoid_copy_actor_info branch July 26, 2019 03:17
simon-mo added a commit that referenced this pull request Aug 2, 2019
@simon-mo
Revert "avoid copying ActorTableData when NodeMananger updates an act…
00beb50 
…or to GCS (#5244)"

This reverts commit 6f682db.
edoakes pushed a commit to edoakes/ray that referenced this pull request Aug 9, 2019
@micafan @edoakes
avoid copying ActorTableData when NodeMananger updates an actor to GCS (
8d7f4b2 
ray-project#5244)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jovany-wang jovany-wang jovany-wang approved these changes

@zhijunfu zhijunfu zhijunfu approved these changes

@raulchen raulchen raulchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@micafan @AmplabJenkins @raulchen @jovany-wang @zhijunfu