[core][nightly] many_actors regression

[rickyyx]

What happened + What you expected to happen

Pretty sure it's due to #31207

Versions / Dependencies

master

Reproduction script

NA

Issue Severity

None

[rickyyx]

Ah, actually the regression happens before #31207.

The last non-regressed run: https://buildkite.com/ray-project/release-tests-branch/builds/1257#01851d3e-e5fb-46c6-a471-6c499d6dc70e
The first regressed run: https://buildkite.com/ray-project/release-tests-branch/builds/1259#01852c73-f3c3-429c-8a10-ebb90d7dbac7

f17375d [Datasets] Fix import of Literal for map_batches benchmark w/ Python 3.7 (#31188)
c692437 [Datasets] [Hotfix] Remove Arrow 8 upper-bound in ray[data] extra (#31186)
1bfc05a Fix typo in a comment (#31187)
c91f861 [Docs/Tune] "getting data in and out of Tune" doc (#30717)
49b1ed1 [RLlib] RLModule: Implement TorchVectorEncoder. (#29875)
9bb62fb [RLlib] Test SampleBatch.get_interceptor functionality. (#31024)
d28552b [RLlib] Fix tf timeline in ray local mode. (#31139)
608276b Simplify logging configuration. (#30863)
c03c4c6 [Tune] Fix AxSearch search space conversion for fixed list hyperparameters (#31088)
24b6a80 [RLlib] Make ComplexInputNet use ModuleDict to register sub-modules. (#31085)
70b0d03 [RLlib] Fix erroring our when tuning any algo without torch being installed. (#31177)
98fef77 [runtime env] Support python 3.10 for runtime_env conda (#30970)
69b2506 [Dashboard New IA] Add overview cards and collapsible sections to the Dashboard (#31013)
8f49901 [docs] Update use cases to include new blog, cleanup
6334e55 [Doc] Reference ray core patterns in docstrings (#30964)
d57b582 Improving message for access to unknown objects (#28209)
4267ac2 [RLlib] Fix reward collection for OpenSpiel games (#31156)
80d0bc7 [Doc] fix the tune's installation instruction in the getting started example (#30280)
66c79f8 Fix tiny typo (#31007)
1829be0 [Core] make ray.all work again for static analyzers such as mypy (#31132)
f4032d7 [Docs] Fix plasma docs related to arrow (#31172)
f2e955c [RLlib] Split torch and tf release learning tests (and use more cost-efficient machines). (#31061)
5d81cf0 [Docs] Add newsletter to the Ray documentation landing page (#31164)
2049954 [RLlib] Policy mapping fn can not be called with keyword arguments. (#31141)

[rickyyx]

Bisected, and it's due to this PR: d57b582 (#28209)

cc @jjyao

Before the PR: https://console.anyscale.com/o/anyscale-internal/projects/prj_FKRmeV5pA6X72aVscFALNC32/clusters/ses_6zhjii2rza1e7bsry6s87wuq?command-history-section=command_history
This PR: https://console.anyscale.com/o/anyscale-internal/projects/prj_FKRmeV5pA6X72aVscFALNC32/clusters/ses_d41pl1vm6lgbienjn6hivaf9

[rickyyx]

I think the root cause is that we added some additional checking of owner in ray.wait and ray.get in the PR:

ray/src/ray/core_worker/core_worker.cc

Lines 1375 to 1382 in 110cae0

	for (size_t i = 0; i < ids.size(); i++) {
	rpc::Address unused_owner_address;
	auto status = GetOwnerAddress(ids[i], &unused_owner_address);
	if (status.IsObjectUnknownOwner()) {
	ids_stream << ids[i] << " ";
	missing_owners += 1;
	}
	}

This is showing up as extra cost in the many_actors because we called ray.wait with multiple obj refs in a loop:

ray/release/benchmarks/distributed/test_many_actors.py

Lines 34 to 36 in e2da42c

	for _ in tqdm.trange(len(actors)):
	ready, not_ready = ray.wait(not_ready)
	assert ray.get(*ready) is None

---

The flamegraph also confirms this:

• Revert this PR

• With this PR

So we have at least 2 options:

1. Revert this PR change: worse observability as pointed out in the original PR but no regression on ray.wait with large number of obj refs
2. Change the test to not call ray.wait in a loop

cc @scv119

[scv119]

thanks for detailed investigation. I do feel this regression is tolerable since it's all memory access. Also am I understand correct comparing to the added time query owner information; the majority of the time will be spend actually waiting for object to be ready? cc @rkooo567 @iycheng for thought.

[ericl]

Could we also optimize the query? It seems we spend a lot of time constructing some rpc::Address protobuf, when we just want to check if the owner is present.

Also, do we have to do the lookup for every object here?

[rkooo567]

Should we reopen in this case? I don't think we optimized the query from the closed PR?