Use std::string_view for read-only string parameters (#59887)#60781
Use std::string_view for read-only string parameters (#59887)#60781MuhammadSaif700 wants to merge 20 commits intoray-project:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great performance-oriented refactoring that replaces const std::string& with std::string_view for read-only string parameters. The changes are applied consistently across many files. The explicit conversions back to std::string are correctly handled where necessary for storage or for APIs that have not yet been updated. I found one minor case where an unnecessary string construction is performed for a map lookup, which I've commented on. Overall, this is a valuable improvement.
src/ray/core_worker/core_worker.cc
Outdated
| CoreWorker::GetNamedActorHandleLocalMode(const std::string &name) { | ||
| auto it = local_mode_named_actor_registry_.find(name); | ||
| CoreWorker::GetNamedActorHandleLocalMode(std::string_view name) { // Changed: read-only param | ||
| auto it = local_mode_named_actor_registry_.find(std::string(name)); // Convert for map lookup |
There was a problem hiding this comment.
The absl::flat_hash_map supports transparent lookups, allowing you to use std::string_view for lookups on a map with std::string keys without creating a temporary std::string. The explicit conversion to std::string here is unnecessary and introduces a performance regression compared to the original code, which goes against the goal of this PR.
auto it = local_mode_named_actor_registry_.find(name); // Convert for map lookupThere was a problem hiding this comment.
Pull request overview
This PR refactors function parameters in core_worker and reference_counter components from const std::string& to std::string_view for read-only string parameters to improve performance by avoiding unnecessary string copies.
Changes:
- Updated 40+ function signatures to use
std::string_viewinstead ofconst std::string& - Added explicit
std::string()conversions where strings are stored in member variables or passed to APIs requiringstd::string - Added
#include <string_view>headers to relevant files
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ray/core_worker/reference_counter_interface.h | Updated interface methods to accept std::string_view for call_site and spilled_url parameters |
| src/ray/core_worker/reference_counter.h | Updated class method declarations to match interface changes |
| src/ray/core_worker/reference_counter.cc | Implemented string_view parameters with proper conversions to std::string for storage and constructor calls |
| src/ray/core_worker/core_worker.h | Updated 20+ method signatures including TaskCounter and CoreWorker methods to use string_view |
| src/ray/core_worker/core_worker.cc | Implemented string_view parameters with proper conversions for storage, protobuf, and map lookups |
| src/ray/common/task/task_spec.h | Updated TaskSpecification constructor to accept string_view with conversion for base class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ray/core_worker/core_worker.cc
Outdated
| std::string_view type, // Changed: read-only param | ||
| std::string_view error_message, // Changed: read-only param |
There was a problem hiding this comment.
The function signature in the implementation file uses std::string_view for type and error_message parameters, but the declaration in core_worker.h (lines 830-831) still uses const std::string &. This mismatch will cause compilation errors. The header file declaration must be updated to match this implementation.
| std::string_view type, // Changed: read-only param | |
| std::string_view error_message, // Changed: read-only param | |
| const std::string &type, | |
| const std::string &error_message, |
|
|
||
| /// Decrease the local reference count for the ObjectID by one. | ||
| /// | ||
| std::string_view call_site) = 0; // Changed: read-only param |
There was a problem hiding this comment.
The documentation comment for the RemoveLocalReference method appears to have been accidentally removed. The comment "Decrease the local reference count for the ObjectID by one" should be preserved above the RemoveLocalReference declaration to maintain API documentation consistency.
| std::string_view call_site) = 0; // Changed: read-only param | |
| std::string_view call_site) = 0; // Changed: read-only param | |
| /// Decrease the local reference count for the ObjectID by one. |
MuhammadSaif700
force-pushed
the
use-string-view
branch
from
9920000 to
81a9ab2
Compare
- Split long default parameter lines for SubmitTask - Align CreateActor comment properly - Reformat SubmitActorTask signature - Reformat DeserializeAndRegisterActorHandle signature - Reformat CreateProfileEvent signature
…_spec files - Reformat function signatures with proper line breaks and parameter alignment - Fix comment alignment for consistency - Split long function call arguments properly - Apply fixes to core_worker.h, core_worker.cc, and task_spec.h
… parameters - Convert string_view to std::string when calling functions that expect const std::string& - Fix Disconnect, ForceExit, PushError, IsRuntimeEnvInfoEmpty, cache Put - Fix SetCommonTaskSpec, SetNormalTaskSpec, SetActorCreationTaskSpec, SetActorTaskSpec - Fix ActorHandle constructor and ProfileEvent constructor calls
- Split Disconnect() arguments across lines - Split RequestShutdown() arguments across lines - Split PushError() arguments across lines - Split IsRuntimeEnvInfoEmpty() argument across lines - Split cache Put() arguments across lines - Split SetNormalTaskSpec() with opening paren break - Split SetActorTaskSpec() with opening paren break - Split ProfileEvent constructor arguments across lines
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…oImpl Convert string_view to std::string once and reuse, instead of creating 3 separate std::string objects. This follows the 'convert once, reuse' pattern and reduces heap allocations in the task-submission hot path. Addresses cursor bot review feedback.
1 participant
Description
Refactors read-only string parameters from
const std::string&tostd::string_viewinsrc/ray/core_worker/andsrc/ray/common/task/to improve performance by avoiding unnecessary string copies.Related issues
Fixes #59887
Additional information
Changes:
#include <string_view>headers where neededstd::string()conversions only where strings are stored or passed to protobufFiles modified:
src/ray/core_worker/core_worker.h/ccsrc/ray/core_worker/reference_counter_interface.hsrc/ray/core_worker/reference_counter.h/ccsrc/ray/common/task/task_spec.hBackward compatible: Existing code continues to work as
std::string_viewaccepts string literals,std::string, and C-strings.