perf: optimize startup time by reading scale factor from config#206
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces a blocking D-Bus call to XSettings1 with reading a persisted scale factor from DConfig in the X11 session initialization path, adds basic validation/logging, and updates the file copyright years. Sequence diagram for X11 scale factor retrieval during session startupsequenceDiagram
participant EnvironmentsManager
participant DConfigFactory as DConfig
participant DConfigObject as DConfigConfig
EnvironmentsManager->>DConfigFactory: create(org.deepin.dde.daemon, org.deepin.XSettings)
DConfigFactory-->>EnvironmentsManager: DConfigConfig
EnvironmentsManager->>DConfigObject: isValid()
alt [config is valid]
EnvironmentsManager->>DConfigObject: value(scale-factor, 1.0)
DConfigObject-->>EnvironmentsManager: scaleFactor
EnvironmentsManager->>EnvironmentsManager: [validate scaleFactor > 0]
else [config not valid]
EnvironmentsManager->>EnvironmentsManager: [fallback scaleFactor = 1.0]
end
EnvironmentsManager->>DConfigObject: deleteLater()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When the DConfig lookup fails or returns the default value, consider falling back to the previous XSettings D-Bus path to preserve behavior for fresh or misconfigured environments.
- The
qInfo()log for the scale factor will run on every session startup; consider downgrading toqDebug()or adding a verbosity guard to avoid noisy logs in normal operation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When the DConfig lookup fails or returns the default value, consider falling back to the previous XSettings D-Bus path to preserve behavior for fresh or misconfigured environments.
- The `qInfo()` log for the scale factor will run on every session startup; consider downgrading to `qDebug()` or adding a verbosity guard to avoid noisy logs in normal operation.
## Individual Comments
### Comment 1
<location path="src/dde-session/environmentsmanager.cpp" line_range="85-88" />
<code_context>
+ // 此时获取的值可能为默认值 1.0。但为了避免 D-Bus 同步调用阻塞启动流程(阻塞时间600ms左右),此处优先使用配置中的持久化值。
+ auto config = DTK_CORE_NAMESPACE::DConfig::create("org.deepin.dde.daemon", "org.deepin.XSettings");
+ if (config && config->isValid()) {
+ scaleFactor = config->value("scale-factor", 1.0).toDouble();
+ qInfo() << "scale factor: " << scaleFactor;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Reconsider using qInfo for per-session scale-factor logging.
This will log on every session startup and may add unnecessary noise to production logs. If this is mainly for debugging, consider using `qDebug` or only logging when the value changes or is invalid to avoid log spam.
```suggestion
if (config && config->isValid()) {
scaleFactor = config->value("scale-factor", 1.0).toDouble();
qDebug() << "scale factor:" << scaleFactor;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Read the scale factor from DConfig instead of using a blocking D-Bus call to XSettings1. This avoids a synchronous wait (around 600ms) during session initialization on X11. 通过读取 DConfig 配置获取缩放比例,代替原本阻塞的 D-Bus 调用。 这避免了 X11 环境下 session 初始化过程中约 600ms 的同步等待时间。 Log: optimize startup time by reading scale factor from config Change-Id: I29dcb24d8cf1f2d5f63d5c70907c399ce0bff328
3c349c7 to
a8ddc4b
Compare
deepin pr auto review你好!我是CodeGeeX。我已仔细审查了你提供的 Git Diff。本次修改主要是将 X11 环境下获取缩放比例的方式从 D-Bus 同步调用 改为了 读取 DConfig 持久化配置,以避免阻塞启动流程,这是一个很好的性能优化方向。 不过,代码在语法逻辑、性能、代码质量和安全性方面还有进一步优化的空间。以下是我的详细审查意见: 1. 语法与逻辑
2. 代码性能
3. 代码质量
4. 代码安全
改进后的代码建议综合以上意见,我为你重构了这部分代码: // 建议在文件头部或类内部定义常量,避免魔法字符串
namespace {
const QString kXSettingsConfigAppId = "org.deepin.dde.daemon";
const QString kXSettingsConfigName = "org.deepin.XSettings";
const QString kScaleFactorKey = "scale-factor";
}
void EnvironmentsManager::createGeneralEnvironments()
{
// ... 前面的代码保持不变 ...
double scaleFactor = 1.0;
if (sessionType == "x11") {
// FIXME: 首次启动桌面时 Xsettings可能尚未计算出推荐缩放比例(由 xsettings 服务计算),
// 此时获取的值可能为默认值 1.0。但为了避免 D-Bus 同步调用阻塞启动流程(阻塞时间600ms左右),此处优先使用配置中的持久化值。
// 建议:如果该函数可能被多次调用,将 config 提升为类成员变量以复用,避免反复 I/O
auto config = DTK_CORE_NAMESPACE::DConfig::create(kXSettingsConfigAppId, kXSettingsConfigName);
if (config && config->isValid()) {
bool ok = false;
double configScale = config->value(kScaleFactorKey, 1.0).toDouble(&ok);
if (ok && configScale >= 1.0) {
scaleFactor = configScale;
} else {
// 显式处理转换失败或非法值的情况
qWarning() << "Invalid scale-factor value from DConfig, fallback to 1.0. Value:" << config->value(kScaleFactorKey);
scaleFactor = 1.0;
}
qInfo() << "Read scale factor from DConfig:" << scaleFactor;
} else {
// 必须添加失败日志,否则配置系统异常时极难排查
qWarning() << "Failed to create or validate DConfig for XSettings, using default scale factor 1.0";
}
// 立即释放资源,如果提升为成员变量则不需要此处手动释放
if (config) {
// 优先考虑直接 delete config; 如果 DConfig 的实现允许且没有挂载信号槽。
// 如果必须使用 deleteLater,确保不会引起事件循环堆积
config->deleteLater();
}
} else {
// TODO: wayland环境下在这里通过xsettings获取是不合理的,此时xsettings服务还没有启动,并且也无法启动(没有DISPLAY,xwayland此时没有启动)
}
// ... 后续使用 scaleFactor 的代码 ...
}总结你本次修改的出发点非常好,解决了启动阻塞的痛点。主要需要关注的是异常分支的日志补充(这对后期维护至关重要),以及考虑是否需要将 DConfig 实例化过程复用以进一步提升性能。希望这些建议对你有所帮助! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mhduiy, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Read the scale factor from DConfig instead of using a blocking D-Bus call to XSettings1. This avoids a synchronous wait (around 600ms) during session initialization on X11.
通过读取 DConfig 配置获取缩放比例,代替原本阻塞的 D-Bus 调用。
这避免了 X11 环境下 session 初始化过程中约 600ms 的同步等待时间。
Log: optimize startup time by reading scale factor from config
Change-Id: I29dcb24d8cf1f2d5f63d5c70907c399ce0bff328
Summary by Sourcery
Optimize session startup by reading the display scale factor from persisted configuration instead of a blocking XSettings D-Bus call on X11.
Enhancements: