Skip to content

Fix default send latency#173

Open
QDelta wants to merge 1 commit intoaliyun:masterfrom
QDelta:master
Open

Fix default send latency#173
QDelta wants to merge 1 commit intoaliyun:masterfrom
QDelta:master

Conversation

@QDelta
Copy link

@QDelta QDelta commented Sep 1, 2025

Do not multiply send_lat by 1000 on default path as it's already ns there.

@CLAassistant
Copy link

CLAassistant commented Sep 1, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the send latency calculation where the default send latency value was being incorrectly multiplied by 1000. The PR description states that send_lat is already in nanoseconds on the default path and should not be multiplied.

Changes:

  • Moved the multiplication by 1000 to only occur when the AS_SEND_LAT environment variable is set (converting from microseconds to nanoseconds)
  • Removed the unconditional multiplication that was incorrectly applied to both the default value and environment variable values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

send_lat = 1000 * std::stoi(send_lat_env);
} catch (const std::invalid_argument& e) {
NcclLog->writeLog(NcclLogLevel::ERROR,"send_lat set error");
exit(-1);
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception handling for std::stoi only catches std::invalid_argument but not std::out_of_range, which can also be thrown when the converted value would fall out of the range of int. Consider catching both exceptions or using a catch-all for std::exception to handle both cases consistently.

Suggested change
exit(-1);
exit(-1);
} catch (const std::out_of_range& e) {
NcclLog->writeLog(NcclLogLevel::ERROR,"send_lat set error");
exit(-1);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants