[runtime env] Short-circuit protobuf serialization/deserialization for empty runtime envs#21788
Conversation
src/ray/core_worker/core_worker.cc
Outdated
| std::shared_ptr<rpc::RuntimeEnv> parent = nullptr; | ||
| if (options_.worker_type == WorkerType::DRIVER) { | ||
| if (serialized_runtime_env == "") { | ||
| if (serialized_runtime_env == "{}") { |
There was a problem hiding this comment.
Is it possible to be "" as well in some cases? Should we check both?
There was a problem hiding this comment.
Good point, I only observed "{}" but it's probably better to be safe and also check "". I've updated the PR.
| std::shared_ptr<rpc::RuntimeEnv> parent = nullptr; | ||
| if (options_.worker_type == WorkerType::DRIVER) { | ||
| if (serialized_runtime_env == "") { | ||
| if (serialized_runtime_env == "" || serialized_runtime_env == "{}") { |
There was a problem hiding this comment.
Thinking now, can we just create a util function? Like IsRuntimeEnvEmpty(serialized_runtime_env) and use it all across the codebase
There was a problem hiding this comment.
This doesn't need to block this PR
There was a problem hiding this comment.
Agree, we should clean this up.
|
Results: |
|
Seems to be consistent with jiajun. but there’s still 300-400 seconds regression.. |
|
I believe it is good enough to unblock the release though.. (about 20%). Thoughts @iycheng ? |
SongGuyang
left a comment
There was a problem hiding this comment.
LGTM. Thank you for this fix!
|
Hey @architkulkarni can you make sure to follow up on #21788 (comment)? |
…r empty runtime envs (#21788) * Fix check for empty runtime_env ("{}", not "") * also check for ""
ray-project#22129) Followup from ray-project#21788. Previously we had a lot of `serialized_runtime_env == "{}" || serialized_runtime_env == ""` scattered around the C++ code; this PR puts this in a helper function.
Why are these changes needed?
stress_test_many_taskshad a performance regression.A test PR attributed a significant part of the performance loss to
OverrideTaskOrActorRuntimeEnv: #21780Inside this function, we short circuit some serialization/deserialization logic if the runtime environment is empty. However, an empty serialized runtime environment is
"{}"(a serialized empty JSON dict.), but the existing code only checked for equality to"". This PR fixes the check.We should probably wait to merge this until we verify that the
stress_test_many_tasksperformance was improved.Related issue number
Partially addresses #21760
Checks
scripts/format.shto lint the changes in this PR.