[core][1/N] Avoid unnecessary deserialization/serialization of TaskId#53577
[core][1/N] Avoid unnecessary deserialization/serialization of TaskId#53577edoakes merged 2 commits intoray-project:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new GetTaskIdBinary() method on TaskSpecification to avoid unnecessary deserialization (TaskId()) followed by re-serialization (Binary()), and replaces existing callsites accordingly.
- Added
TaskSpecification::GetTaskIdBinary()to return the raw binary task ID (or a nil ID if unset). - Updated RPC serializers and submitter logic to use
GetTaskIdBinary()instead ofTaskId().Binary(). - Adjusted unit tests to expect
GetTaskIdBinary()in place ofTaskId().Binary().
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ray/common/task/task_spec.h | Declares GetTaskIdBinary() on TaskSpecification. |
| src/ray/common/task/task_spec.cc | Implements GetTaskIdBinary(). |
| src/ray/gcs/pb_util.h | Uses GetTaskIdBinary() in FillTaskInfo overloads. |
| src/ray/core_worker/transport/normal_task_submitter.cc | Replaces TaskId().Binary() with GetTaskIdBinary(). |
| src/ray/core_worker/transport/actor_task_submitter.cc | Same replacement in actor submitter. |
| src/ray/core_worker/task_manager.cc | Uses new method in stats filling. |
| src/ray/core_worker/core_worker.cc | Updates running task ID serialization. |
| src/ray/core_worker/test/normal_task_submitter_test.cc | Updates expectations in kill-task tests. |
| src/ray/core_worker/test/actor_task_submitter_test.cc | Updates retries in actor task test. |
Comments suppressed due to low confidence (3)
src/ray/common/task/task_spec.h:296
- Consider adding dedicated unit tests for
GetTaskIdBinary(), covering both the non-empty and empty (nil) cases to ensure it returns the expected binary string.
std::string GetTaskIdBinary() const;
src/ray/common/task/task_spec.h:296
- [nitpick] The comment for
GetTaskIdBinary()could note that this method bypasses deserialization, highlighting its performance benefit and intended use overTaskId().Binary().
std::string GetTaskIdBinary() const;
src/ray/common/task/task_spec.h:296
- [nitpick] This accessor is small and called frequently—marking it
inlinein the header could avoid an out-of-line call and improve performance in hot paths.
std::string GetTaskIdBinary() const;
src/ray/common/task/task_spec.cc
Outdated
There was a problem hiding this comment.
same as other PR -- the existing method is TaskId, not GetTaskId. keep naming consistent.
There was a problem hiding this comment.
Update TaskId to GetTaskId to keep naming consistent #53574 (comment).
If you prefer TaskId to GetTaskId, feel free to let me know.
There was a problem hiding this comment.
GetTaskId sounds better to me
Though now we have more inconsistency because half of the TaskSpecification getters start with Get and half don't :)
Can you follow up in a separate PR to update all of them?
There was a problem hiding this comment.
Double checked in the Google C++ style and there is no explicit mention of this, but they indicate that getters/setters can be named with snake_case like variable names and in the example they give, there is no get or Get prefix.
I don't have a strong opinion here because I am not a C++ expert. @dayshah is there a standard idiom we should be following?
There was a problem hiding this comment.
they indicate that getters/setters can be named with snake_case like variable names and in the example they give, there is no get or Get prefix.
How about we keep using TaskId and rename GetTaskIdBinary to TaskIdBinary? I can remove the prefix Get from GetArgObjectId and GetArgObjectIdBinary to keep the consistency.
The Get prefix doesn’t affect readability for me.
There was a problem hiding this comment.
ya usually you don't want the Get prefix. norm is a getter/setter with the same name one is const, one is non const
4d12675 to
8dbb0ee
Compare
…#53577) ```cpp task_spec.TaskId().Binary() ``` * `TaskId()` deserializes binary to TaskId. * `Binary()` serializes `TaskId` to binary. This PR: * renames `TaskId` to `GetTaskId` * adds `GetTaskIdBinary` --------- Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
…#53577) ```cpp task_spec.TaskId().Binary() ``` * `TaskId()` deserializes binary to TaskId. * `Binary()` serializes `TaskId` to binary. This PR: * renames `TaskId` to `GetTaskId` * adds `GetTaskIdBinary` --------- Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Why are these changes needed?
TaskId()deserializes binary to TaskId.Binary()serializesTaskIdto binary.This PR:
TaskIdtoGetTaskIdGetTaskIdBinaryRelated issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.