Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Added a toggle enableDisplayCustomTypeInfo aimed at further enhancing performance #53

Merged
merged 8 commits into from
Feb 23, 2024
Merged

feat: Added a toggle enableDisplayCustomTypeInfo aimed at further enhancing performance #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7ca4b51
feat: Added a toggle enableDisplayCustomTypeInfo aimed at further enh…
zhangjiequan Feb 22, 2024
c596cf2
Merge branch 'master' into feat/extend_all_lua_types_for_pr
zhangjiequan Feb 22, 2024
0832024
style: Update method names to follow PascalCase convention
zhangjiequan Feb 22, 2024
8f1d78d
Revert "style: Update method names to follow PascalCase convention"
zhangjiequan Feb 23, 2024
1e94fc1
Revert "feat: Added a toggle enableDisplayCustomTypeInfo aimed at fur…
zhangjiequan Feb 23, 2024
9b97cec
feat: Added a bool displayCustomTypeInfo aimed at further enhancing p…
zhangjiequan Feb 23, 2024
fb24157
style: Replace spaces with tabs for consistent indentation across files
zhangjiequan Feb 23, 2024
e8bfb0e
style: Replace spaces with tabs for consistent indentation across files
zhangjiequan Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions emmy_debugger/include/emmy_debugger/debugger/emmy_debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,5 +143,6 @@ class Debugger: public std::enable_shared_from_this<Debugger>

Arena<Variable> *arenaRef;

bool displayCustomTypeInfo;
std::bitset<LUA_NUMTAGS> registeredTypes;
};
40 changes: 21 additions & 19 deletions emmy_debugger/src/debugger/emmy_debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ Debugger::Debugger(lua_State *L, EmmyDebuggerManager *manager)
running(false),
skipHook(false),
blocking(false),
arenaRef(nullptr) {
arenaRef(nullptr),
displayCustomTypeInfo(false) {
}

Debugger::~Debugger() {
Expand Down Expand Up @@ -371,7 +372,7 @@ void Debugger::GetVariable(lua_State *L, Idx<Variable> variable, int index, int
variable->valueType = type;

if (queryHelper) {
if (type >= 0 && type < registeredTypes.size() && registeredTypes.test(type)
if (displayCustomTypeInfo && type >= 0 && type < registeredTypes.size() && registeredTypes.test(type)
Copy link
Member

Choose a reason for hiding this comment

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

这里的所有判断可以只保留registeredTypes.test(type), 因为lua_type的返回值是确定的不会受外部影响增加,所以边界判断毫无意义

Copy link
Contributor Author

@zhangjiequan zhangjiequan Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

可能的返回值-1

首先，关于您提到的lua_type的返回值范围的问题，它是否可能返回-1？
有定义：#define LUA_TNONE (-1)，
这是在对应index没有元素时会发生的，因此确实需要考虑这种情况？

仅判断registeredTypes.set的可行性

如果我们确信lua_type的返回值严格在-1至8的范围内，我们确实可以考虑简化判断逻辑以提升性能。

然而，这种性能优化会以牺牲代码可读性为代价。这个tradeoff是否值得，我认为是有待商榷的。

以下是 仅判断registeredTypes.set 方案的示意代码：

std::bitset<LUA_NUMTAGS+1> registeredTypes;

bool Debugger::RegisterTypeName(const std::string& typeName, std::string& err) {
//...
    registeredTypes.set(type+1);
    return true;
}

void Debugger::GetVariable(lua_State *L, Idx<Variable> variable, int index, int depth, bool queryHelper) {
//...
        if (registeredTypes.test(type+1)
            && manager->extension.QueryVariableCustom(L, variable, typeName, index, depth)) {
            return;
        }
//...

性能比较：单一判断 vs 组合判断

当enableDisplayCustomTypeInfo被禁用且没有注册任何类型时，
我们比较仅使用registeredTypes.test(type+1)与enableDisplayCustomTypeInfo && registeredTypes.test(type+1)两种方式的性能。

针对后者，根据短路求值的特性，只需要考虑enableDisplayCustomTypeInfo的开销。

即我们对比两者的话，需要对比registeredTypes.test(type+1)与enableDisplayCustomTypeInfo，显然后者能提供更好的性能，因为它避免了不必要的类型检查，只需要进行最直接的布尔值检查。而前者的类型检查，涉及函数调用等开销，性能稍差。

所以我认为保留enableDisplayCustomTypeInfo作为一个配置选项是有价值的，它为性能优化提供了额外的灵活性。

Copy link
Member

Choose a reason for hiding this comment

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

这里的性能其实没那么重要, 主要是写的太冗长了, 另外就算你要保留enableDisplayCustomTypeInfo的设计, 那么也不应该提供独立的函数去开启这种功能, 因为registerType 和enable这两个函数一定是一起调用的, 所以可以直接在registerType内设置enableDisplayCustomTypeInfo = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我充分理解并尊重你的看法。

  1. 确实，我们在代码性能与简洁性之间往往需要找到平衡。就本例而言，我们有时倾向于追求影响尽可能的要小、性能要足够的高(你在Supports customizing the display of all data types in Lua on the project side(支持在项目侧自定义Lua中所有类型数据的展示方式) by zhangjiequan · Pull Request #51 · EmmyLua/EmmyLuaDebugger中提出的)，而有时我们可能会觉得这里的性能其实没那么重要。这的确需要我们根据具体情况进行分析和权衡。
  2. 我支持保留 emmy_debugger 中 displayCustomTypeInfo 的设计。
  3. 关于你提出的不为用户单独提供 enableDisplayCustomTypeInfo 方法，而是在registerType中直接激活这一功能的建议，我认为这是一个简化用户操作的好方法。我会进行相应的修改。

&& manager->extension.QueryVariableCustom(L, variable, typeName, index, depth)) {
return;
}
Expand Down Expand Up @@ -1430,24 +1431,25 @@ void Debugger::ExecuteOnLuaThread(const Executor &exec) {
}

int Debugger::GetTypeFromName(const char* typeName) {
if (strcmp(typeName, "nil") == 0) return LUA_TNIL;
if (strcmp(typeName, "boolean") == 0) return LUA_TBOOLEAN;
if (strcmp(typeName, "lightuserdata") == 0) return LUA_TLIGHTUSERDATA;
if (strcmp(typeName, "number") == 0) return LUA_TNUMBER;
if (strcmp(typeName, "string") == 0) return LUA_TSTRING;
if (strcmp(typeName, "table") == 0) return LUA_TTABLE;
if (strcmp(typeName, "function") == 0) return LUA_TFUNCTION;
if (strcmp(typeName, "userdata") == 0) return LUA_TUSERDATA;
if (strcmp(typeName, "thread") == 0) return LUA_TTHREAD;
return -1; // 未知类型
if (strcmp(typeName, "nil") == 0) return LUA_TNIL;
if (strcmp(typeName, "boolean") == 0) return LUA_TBOOLEAN;
if (strcmp(typeName, "lightuserdata") == 0) return LUA_TLIGHTUSERDATA;
if (strcmp(typeName, "number") == 0) return LUA_TNUMBER;
if (strcmp(typeName, "string") == 0) return LUA_TSTRING;
if (strcmp(typeName, "table") == 0) return LUA_TTABLE;
if (strcmp(typeName, "function") == 0) return LUA_TFUNCTION;
if (strcmp(typeName, "userdata") == 0) return LUA_TUSERDATA;
if (strcmp(typeName, "thread") == 0) return LUA_TTHREAD;
return -1; // 未知类型
}

bool Debugger::RegisterTypeName(const std::string& typeName, std::string& err) {
int type = GetTypeFromName(typeName.c_str());
if (type == -1) {
err = "Unknown type name: " + typeName;
return false;
}
registeredTypes.set(type);
return true;
int type = GetTypeFromName(typeName.c_str());
if (type == -1) {
err = "Unknown type name: " + typeName;
return false;
}
displayCustomTypeInfo = true;
zhangjiequan marked this conversation as resolved.
Show resolved Hide resolved
registeredTypes.set(type);
return true;
}
6 changes: 3 additions & 3 deletions emmy_debugger/src/debugger/extension_point.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ bool ExtensionPoint::QueryVariableGeneric(lua_State *L, Idx<Variable> variable,
result = lua_toboolean(L, -1);
} else {
const auto err = lua_tostring(L, -1);
printf("query error in %s: %s\n", queryFunction, err);
printf("query error in %s: %s\n", queryFunction, err);
}
}
}
Expand All @@ -150,11 +150,11 @@ bool ExtensionPoint::QueryVariableGeneric(lua_State *L, Idx<Variable> variable,
}

bool ExtensionPoint::QueryVariable(lua_State *L, Idx<Variable> variable, const char *typeName, int object, int depth) {
return QueryVariableGeneric(L, variable, typeName, object, depth, "queryVariable");
return QueryVariableGeneric(L, variable, typeName, object, depth, "queryVariable");
}

bool ExtensionPoint::QueryVariableCustom(lua_State *L, Idx<Variable> variable, const char *typeName, int object, int depth) {
return QueryVariableGeneric(L, variable, typeName, object, depth, "queryVariableCustom");
return QueryVariableGeneric(L, variable, typeName, object, depth, "queryVariableCustom");
}

lua_State *ExtensionPoint::QueryParentThread(lua_State *L) {
Expand Down