Skip to content

[SOT][FasterGuard] add ExprNodeBase cleanup func #72552

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

[SOT][FasterGuard] add ExprNodeBase cleanup func #72552

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d950d44
[SOT][FasterGuard] add `ExprNodeBase` cleanup func
gouzil Apr 29, 2025
cefae84
fix value is null
gouzil Apr 30, 2025
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
25 changes: 23 additions & 2 deletions paddle/fluid/pybind/sot/guards.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ static inline PyObject* PyObject_CallOneArg(PyObject* func, PyObject* arg) {
} \
}

// ExprNodeBase delayed cleaning
#define DELAYED_CLEAN(clean_py_obj_) \
{ \
for (auto it = clean_py_obj_.begin(); it != clean_py_obj_.end();) { \
bool should_erase = true; \
if (*it) { \
Py_DECREF(*it); \
should_erase = (Py_REFCNT(*it) <= 0); \
} \
it = should_erase ? clean_py_obj_.erase(it) : ++it; \
} \
Copy link
Preview

Copilot AI Apr 29, 2025

Choose a reason for hiding this comment

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

Calling Py_REFCNT on a PyObject immediately after a Py_DECREF may cause undefined behavior if the object is deallocated when its reference count reaches zero. Consider updating the cleanup logic to avoid accessing deallocated memory.

Suggested change
bool should_erase = true; \
if (*it) { \
Py_DECREF(*it); \
should_erase = (Py_REFCNT(*it) <= 0); \
} \
it = should_erase ? clean_py_obj_.erase(it) : ++it; \
} \
if (*it) { \
PyObject* temp = *it; \
Py_DECREF(temp); \
it = clean_py_obj_.erase(it); \
} else { \
++it; \
} \

Copilot uses AI. Check for mistakes.

}

static inline bool PyObject_Equal(PyObject* a, PyObject* b) {
if (a == b) {
return true;
Expand Down Expand Up @@ -252,25 +265,33 @@ std::string GlobalVarExprNode::stringify(int indent) {

PyObject* AttributeExprNode::eval(FrameProxy* frame) {
PyObject* var = var_expr_->eval(frame);
return PyObject_GetAttrString(var, attr_name_.c_str());
auto res = PyObject_GetAttrString(var, attr_name_.c_str());
clean_py_obj_.push_back(res);
return res;
}
std::string AttributeExprNode::stringify(int indent) {
std::stringstream ss;
ss << var_expr_->stringify() << "." << attr_name_;
return ss.str();
}

void AttributeExprNode::cleanup() { DELAYED_CLEAN(clean_py_obj_); }

PyObject* ItemExprNode::eval(FrameProxy* frame) {
PyObject* var = var_expr_->eval(frame);
PyObject* key = key_expr_->eval(frame);
Comment on lines 279 to 280
Copy link
Member

Choose a reason for hiding this comment

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

var_expr_ 和 key_expr_ 不会清理么？

假如 a.b.c.d.e.f，最终 a.b、a.b.c、a.b.c.d、a.b.c.d.e 是不是还是会被 hold 住？

另外，引用 obj 使用 set 管理是否会出现 a.b[a.b]，这里 a.b incref 了两次，然后只清理一次的问题（现在还不是问题，因为现在一次都没清理，但一旦修改后就会面临这个问题）

return PyObject_GetItem(var, key);
auto res = PyObject_GetItem(var, key);
clean_py_obj_.push_back(res);
return res;
}
std::string ItemExprNode::stringify(int indent) {
std::stringstream ss;
ss << var_expr_->stringify() << "[" << key_expr_->stringify() << "]";
return ss.str();
}

void ItemExprNode::cleanup() { DELAYED_CLEAN(clean_py_obj_); }

PyObject* BinaryExprNode::eval(FrameProxy* frame) {
PyObject* lhs = lhs_->eval(frame);
PyObject* rhs = rhs_->eval(frame);
Expand Down
6 changes: 6 additions & 0 deletions paddle/fluid/pybind/sot/guards.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ class ExprNodeBase : public GuardTreeNodeBase,
public:
virtual PyObject* eval(FrameProxy* frame) = 0;
virtual ~ExprNodeBase() = default;
virtual void cleanup() {}
};

class ConstantExprNode : public ExprNodeBase {
Expand Down Expand Up @@ -345,10 +346,12 @@ class AttributeExprNode : public ExprNodeBase {

PyObject* eval(FrameProxy* frame) override;
std::string stringify(int indent = 0) override;
void cleanup() override;

private:
std::shared_ptr<ExprNodeBase> var_expr_;
std::string attr_name_;
std::vector<PyObject*> clean_py_obj_;
};

class ItemExprNode : public ExprNodeBase {
Expand All @@ -359,10 +362,12 @@ class ItemExprNode : public ExprNodeBase {

PyObject* eval(FrameProxy* frame) override;
std::string stringify(int indent = 0) override;
void cleanup() override;

private:
std::shared_ptr<ExprNodeBase> var_expr_;
std::shared_ptr<ExprNodeBase> key_expr_;
std::vector<PyObject*> clean_py_obj_;
};

class BinaryExprNode : public ExprNodeBase {
Expand Down Expand Up @@ -544,6 +549,7 @@ class CheckGuardNode : public GuardNodeBase {
ret = lookup_next(frame);
}
for (size_t i = 0; i < N; ++i) {
exprs[i]->cleanup();
if (values[i]) {
Py_DECREF(values[i]);
}
Expand Down
Loading