Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Nov 3, 2025

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).

int
_Py_uop_promote_to_constant_pool(JitOptContext *ctx, PyObject *obj)
{
return PyList_Append(ctx->constant_pool, obj);
Copy link
Contributor

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)?

Copy link
Member Author

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.

Copy link
Contributor

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.

@Zheaoli
Copy link
Contributor

Zheaoli commented Nov 17, 2025

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

@Fidget-Spinner
Copy link
Member Author

I think we don't limit the pool size, right? Will this cause a memory leak or not?

If we're careful about what we add to the pool, there won't be a memory leak.

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 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.

@Zheaoli
Copy link
Contributor

Zheaoli commented Nov 17, 2025

I think we don't limit the pool size, right? Will this cause a memory leak or not?

If we're careful about what we add to the pool, there won't be a memory leak.

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 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants