[Feat] Add Cache IO aggregation for Ascend#1009
Conversation
6f5fc11 to
c1efb06
Compare
c1efb06 to
f40e380
Compare
|
|
||
| void AscendShardIOAggregator::Cleanup() noexcept | ||
| { | ||
| if (deviceId_ >= 0) { (void)aclrtSetDevice(deviceId_); } |
There was a problem hiding this comment.
💡 Suggestion: After aclrtSetDevice, the code should check for errors before proceeding. The current implementation checks this, which is good.
|
|
||
| Status FftsD2DDispatcher::AddMemcpy(void* dst, const void* src, size_t size) | ||
| { | ||
| if (completed_) { return Status::Error("FFTS dispatcher already launched"); } |
There was a problem hiding this comment.
💡 Suggestion: The completed_ flag prevents adding new copies after launch. This is a good safety check. Consider also resetting this flag in Reset() to allow reuse of the dispatcher.
f40e380 to
d48eb09
Compare
| public: | ||
| Status Setup(const Config&) override | ||
| { | ||
| return Status::InvalidParam("Cache IO aggregation is not compiled"); |
There was a problem hiding this comment.
💡 Suggestion: The UnavailableAggregatedIOExecutor returns Status::InvalidParam for all methods when IO aggregation is not compiled. Since this is not a parameter validation error (the parameters may be perfectly valid), consider using Status::Unsupported() instead. This better communicates that the feature is simply not available in this build configuration.
The Status::Unsupported() method is available in ucm/shared/infra/status/status.h and maps to error code -50008, which the troubleshooting documentation describes as "Feature or operation not supported in current context" - exactly matching this scenario.
Summary
git diff --check upstream/develop..HEAD