-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-140928: Add a constant pool to all executors; Contant propagate using the pool #140968
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
base: main
Are you sure you want to change the base?
Conversation
| int | ||
| _Py_uop_promote_to_constant_pool(JitOptContext *ctx, PyObject *obj) | ||
| { | ||
| return PyList_Append(ctx->constant_pool, obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add an assert here, like assert(ctx->contstant_pool)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyList_Append itself already checks that it's a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but no. If you pass a NULL pointer to PyList_Append it crashes.
I'm sure there is no chances to get NULL pointer here. But I believe extra assert will not hurt.
|
I'm not sure my thought is right. Correct me plz if I'm wrong. I think we don't limit the pool size, right? Will this cause a memory leak or not? And I think this is a append only list for now. I'm not sure we need to think about if the user delete some const variable and we need to add a pop out path |
If we're careful about what we add to the pool, there won't be a memory leak.
If the user deletes a constant variable, the variable will stay alive in the pool for now. This is fine as the pool only contains constants which don't take a lot of memory. |
I see, this is making sense. Thanks for your work!(lol |
This PR promotes constant propagated objects into a pool. Storing them there. We already see this affecting the optimizations in the test suite, as more things are constant propagated away!
This sets up the infrastructure to promote more things in the future should we feel like it (e.g. function, code objects).