[core] Cache runtime env core worker #48928
Open
+61
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: hjiang <[email protected]>
dentiny
force-pushed
the
hjiang/cache-runtime-env-core-worker
branch
2 times, most recently
from
ef4760a
to
ae5543b
Compare
Signed-off-by: hjiang <[email protected]> Signed-off-by: hjiang <[email protected]>
dentiny
force-pushed
the
hjiang/cache-runtime-env-core-worker
branch
from
ae5543b
to
321dc87
Compare
jjyao
reviewed
Nov 26, 2024
From local |
Signed-off-by: hjiang <[email protected]>
Can you test the real release test and see the performance impact? |
rynewang
reviewed
Nov 26, 2024
jjyao
reviewed
Nov 27, 2024
src/ray/core_worker/core_worker.h
Outdated
| /// Serialized runtime info env are cached. | ||
| mutable std::mutex runtime_env_serialization_mutex_; | ||
| /// Maps serialized runtime env to **immutable** deserialized protobuf. | ||
| mutable boost::compute::detail::lru_cache<std::string, |
There was a problem hiding this comment.
Can we temporarily change this back to std::unordered_map so that this PR compiles and we can see the real perf impact on the release test. I'm curious whether the release test will see the same perf improvement as what you observed locally.
There was a problem hiding this comment.
Sure, release run launched: https://buildkite.com/ray-project/release/builds/27207
There was a problem hiding this comment.
Is this with the modification of release test to add env var runtime env?
rynewang
pushed a commit
that referenced
this pull request
Nov 28, 2024
A shared lru cache is needed for my PR: #48928 Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
dentiny
force-pushed
the
hjiang/cache-runtime-env-core-worker
branch
from
fd5b0bd
to
b52df48
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup PR for #48749
This PR caches json and protobuf deserialization for serialized runtime env.
Checked with benchmark (https://github.com/ray-project/ray/blob/master/release/benchmarks/distributed/test_many_tasks.py), verified throughput improves from ~11550 to ~ 13565, which is ~17%.