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.

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

python: Fix GC isn't working correctly. #1888

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

yihong0618
Copy link
Contributor

The uftrace_python_symbol keeps code PyObject in rb tree so GC can not run correctly in the trace process. Fixed the problem by dropping it.

Fixed: #1886

Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! But I'm afraid it won't work well because get_new_sym_addr() always returns a new address even for the same name.

I think you need to compare func_name instead. It'd have little more overhead but probably better to make GC working properly.

python/trace-python.c Show resolved Hide resolved
python/trace-python.c Show resolved Hide resolved
/* just compare pointers of the code object */
if (iter->code == code) {
Py_DECREF(code);
if (iter->addr == new_addr) {
Copy link
Owner

Choose a reason for hiding this comment

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

It should free func_name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@yihong0618 yihong0618 force-pushed the fix_issue_1886 branch 4 times, most recently from 1e0f0ba to efbe17c Compare February 15, 2024 08:12
@yihong0618
Copy link
Contributor Author

Fixed thanks for the review, And use func_name is indeed a better way. and uftrace_py_traverse and uftrace_py_clean is no need either.

python/trace-python.c Show resolved Hide resolved
if (iter->code == code) {
Py_DECREF(code);
/* compare func_name */
if (iter->name == func_name) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we can do this. It should call strcmp().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

python/trace-python.c Show resolved Hide resolved
The uftrace_python_symbol keeps code PyObject in rb tree so GC can
not run correctly in the trace process. Fixed the problem by
dropping it. and change rb compare from code PyObject to func_name

Fixed: namhyung#1886
Signed-off-by: Yi Hong <zouzou0208@gmail.com>
Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

LGTM

@namhyung namhyung merged commit 25b9035 into namhyung:master Feb 17, 2024
3 checks passed
@honggyukim
Copy link
Collaborator

Hi @yihong0618, thanks very much for the fix. I've also found that it fixes the problem I reported.

But you don't have to put my Signed-off-by when I didn't involve the patch itself. I think Reported-by would be better in this case.

@yihong0618
Copy link
Contributor Author

Hi @yihong0618, thanks very much for the fix. I've also found that it fixes the problem I reported.

But you don't have to put my Signed-off-by when I didn't involve the patch itself. I think Reported-by would be better in this case.

learned. will remember that next thanks a lot^_^

@yihong0618 yihong0618 deleted the fix_issue_1886 branch February 18, 2024 12:12
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.

python tracing failure with RuntimeError: Too many open files
3 participants