[xray] Evict tasks from the lineage cache by stephanie-wang · Pull Request #2152 · ray-project/ray

stephanie-wang · 2018-05-29T19:10:30Z

What do these changes do?

This improves the eviction algorithm for the lineage cache. Changes:

Remove the COMMITTED status from the lineage cache. These are tasks that have been committed in the GCS. Instead of keeping these entries around for an indefinite time, tasks are now evicted from the cache as soon as a notification for their commit is received.
Attempt to bound each remote task's uncommitted lineage to a configurable maximum lineage size. This is necessary for actor tasks, where a long chain of tasks may be forwarded to another node for execution. In this case, the tasks will remain in the sending node's lineage cache with status REMOTE and must be evicted. This change periodically evicts these remote tasks by requesting a notification for their commit after the uncommitted lineage exceeds the configured size.
Better checking for requesting/canceling notifications on a task.

…committed lineage

…testing for out-of-order operations

AmplabJenkins · 2018-05-29T19:42:12Z

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

robertnishihara · 2018-05-30T07:14:40Z

jenkins, retest this please

AmplabJenkins · 2018-05-30T08:26:40Z

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

robertnishihara · 2018-05-30T19:39:06Z

src/ray/raylet/lineage_cache.cc

+  } else {
+    return false;
+  }
+}


Purely a style thing, but I think it'd be cleaner to have a single return statement and to do something like

bool task_present = it != subscribed_tasks_.end(); if (task_present) { ... } return task_present;

and similarly for the above block.

robertnishihara · 2018-05-30T20:34:55Z

src/common/state/ray_config.h

1000 feels pretty big to me

Hmm, what do you think is a good size?

robertnishihara · 2018-05-30T21:17:23Z

src/ray/raylet/lineage_cache.cc

+  // NOTE(swang): The number of entries in the uncommitted lineage also
+  // includes local tasks that haven't been committed yet, not just remote
+  // tasks, so this is an overestimate.
+  const auto uncommitted_lineage = GetUncommittedLineage(task_id);


This feels like an expensive operation, especially if we allow the uncommitted lineage to grow to size 1000. And this is called every time we forward a task, right? Will this be an issue?

Yes, it will be called every time we forward a task. It shouldn't be too expensive since it's just walking a tree, but I guess it's hard to say. Would it help if we added a timing statement to GetUncommittedLineage and logged a warning if it exceeds maybe 1ms?

Up to you about the timing statement, I assume it will be visible in the profiler if it's an issue.

Hmm okay, I think it's a good sanity check. I'll run this quickly locally and see how many tasks it takes to get to 1ms.

robertnishihara · 2018-05-30T21:17:37Z

src/ray/raylet/lineage_cache.cc

+  // includes local tasks that haven't been committed yet, not just remote
+  // tasks, so this is an overestimate.
+  const auto uncommitted_lineage = GetUncommittedLineage(task_id);
+  if (uncommitted_lineage.GetEntries().size() > max_lineage_size_) {


What goes wrong if max_lineage_size_ = 0?

Hmm, I think that will basically just try to evict every single entry in the lineage, but the code itself should not break. The tradeoff is basically the lower max_lineage_size_ is, the more often a node will request notifications from the GCS.

robertnishihara · 2018-05-30T21:52:31Z

I think this can be merged when the tests pass. I think for the timing statement either way is ok.

AmplabJenkins · 2018-05-30T22:22:31Z

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

stephanie-wang · 2018-05-30T22:25:21Z

Okay, we can leave out the timing statement for now, but after a quick test, GetUncommittedLineage with size 1000 takes over 1ms, so it probably makes sense to lower that. I'll lower it to 100.

AmplabJenkins · 2018-05-30T23:38:28Z

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

* master: [autoscaler] GCP node provider (ray-project#2061) [xray] Evict tasks from the lineage cache (ray-project#2152) [ASV] Add ray.init and simple Ray benchmarks (ray-project#2166) Re-encrypt key for uploading to S3 from travis to use travis-ci.com. (ray-project#2169) [rllib] Fix A3C PyTorch implementation (ray-project#2036) [JavaWorker] Do not kill local-scheduler-forked workers in RunManager.cleanup (ray-project#2151) Update Travis CI badge from travis-ci.org to travis-ci.com. (ray-project#2155) Implement Python global state API for xray. (ray-project#2125) [xray] Improve flush algorithm for the lineage cache (ray-project#2130) Fix support for actor classmethods (ray-project#2146) Add empty df test (ray-project#1879) [JavaWorker] Enable java worker support (ray-project#2094) [DataFrame] Fixing the code formatting of the tests (ray-project#2123) Update resource documentation (remove outdated limitations). (ray-project#2022) bugfix: use array redis_primary_addr out of its scope (ray-project#2139) Fix infinite retry in Push function. (ray-project#2133) [JavaWorker] Changes to the directory under src for support java worker (ray-project#2093) Integrate credis with Ray & route task table entries into credis. (ray-project#1841)

stephanie-wang added 4 commits May 28, 2018 21:03

Add max lineage size as parameter to LineageCache

6c940a4

Add a regression test to make sure eviction bounds the size of the un…

c67adb5

…committed lineage

Evict a remote task's lineage if its lineage exceeds the configured size

9b79186

Remove COMMITTED status, evict lineage from cache, better checks and …

e051dc5

…testing for out-of-order operations

stephanie-wang requested review from pcmoritz and robertnishihara May 29, 2018 19:10

robertnishihara reviewed May 30, 2018

View reviewed changes

style

0227740

robertnishihara approved these changes May 30, 2018

View reviewed changes

Lower maximum lineage size

42c60bd

robertnishihara merged commit 117107c into ray-project:master May 31, 2018

robertnishihara deleted the evict-lineage-cache branch May 31, 2018 07:24

Conversation

stephanie-wang commented May 29, 2018

What do these changes do?

Uh oh!

AmplabJenkins commented May 29, 2018

Uh oh!

robertnishihara commented May 30, 2018

Uh oh!

AmplabJenkins commented May 30, 2018

Uh oh!

robertnishihara May 30, 2018 • 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!

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!

robertnishihara commented May 30, 2018

Uh oh!

AmplabJenkins commented May 30, 2018

Uh oh!

stephanie-wang commented May 30, 2018

Uh oh!

AmplabJenkins commented May 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

robertnishihara May 30, 2018 •

edited

Loading