python / cpython

The Python programming language
https://www.python.org/
Other
60.06k stars 29.08k forks source link

gh-117139: Convert the evaluation stack to stack refs #118450

Open Fidget-Spinner opened 2 weeks ago

Fidget-Spinner commented 2 weeks ago

Only tags all pointers 0b11 and NULL and immortal stuff as deferred for now.

gvanrossum commented 2 weeks ago

Could we hold off on this until 3.14? It's only a week until feature freeze for 3.13 (at which point main becomes 3.14), and this looks like a lot of churn in a time where we all would like stability to merge things that are actually needed in 3.13.

Fidget-Spinner commented 2 weeks ago

Could we hold off on this until 3.14? It's only a week until feature freeze for 3.13 (at which point main becomes 3.14), and this looks like a lot of churn in a time where we all would like stability to merge things that are actually needed in 3.13.

Alright. I always forget that ten other people are rushing in things at the same time as me. I don't think this will add any value for CPython 3.13 anyways at the moment.

colesbury commented 2 weeks ago

Could we hold off on this until 3.14?

That makes sense. I'll start providing feedback and reviewing this now, but it won't be merged in 3.13.

Fidget-Spinner commented 2 weeks ago

Adding a lot more deferred objects exposes refleaks in the call sequence again. Need to investigate.

markshannon commented 1 week ago

Can we have an issue for this. The linked issue is just about the tagging scheme, but this goes much deeper.

Without an isssue, there is nowhere to discuss design or other higher level problems.

For now, I'll comment on this PR.

markshannon commented 1 week ago

How are generators and coroutines to be handled?

If a deferred reference to an object is held in a suspended generator, and there are no other references to that object, what happens?

bedevere-app[bot] commented 1 week ago

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Fidget-Spinner commented 1 week ago

Can we have an issue for this. The linked issue is just about the tagging scheme, but this goes much deeper.

Please comment on that issue, or we can rename the title to be more inclusive of the current PR.

Fidget-Spinner commented 1 week ago

I have made the requested changes; please review again

Namely:

  1. Manually convert most of the bytecodes.c to stackref, instead of making it magically done by the cases generator.
  2. Fix ownership problems when using steal or borrow.
  3. Tag all pointers 0b11.

To be conservative, this PR only defers immortal objects at the moment and NULL.

bedevere-app[bot] commented 1 week ago

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

Fidget-Spinner commented 1 week ago

Note: I'm aware that certain decref options don't follow the semantics laid out in the issue. For now they're still safe because they will never operate on deferred objects. In PEP 703's plan, (non-immortal) floats and ints will not be deferred. So we're safe for now. However, in the future if we want to defer everything, then those need to be removed https://github.com/python/cpython/issues/118930.

Fidget-Spinner commented 1 week ago

5% performance hit on single-threaded default build: https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20240511-3.14.0a0-144a6fa

markshannon commented 1 week ago

I really think we need a design document (in an issue) that we can discuss before doing more work on this. I will attempt to summarize what I believe to be the design at the moment:

colesbury commented 1 week ago

I really think we need a design document (in an issue)...

I'll add a more detailed summary of the GC changes to:

I don't understand how non-deferred objects are freed, as we cannot safely look at non-deferred pointers

We don't look at non-deferred pointers above stacktop (beyond checking the tag).

To be clear, what we are describing here is an extra step in the GC that scans each thread's stack and ensures that deferred referenced objects from thread's stacks are kept alive. These frames are mostly not tracked and would not otherwise be considered by the GC.

We don't need to do anything special in this step for non-deferred references because those objects are kept alive by their reference count (i.e., computed gc_refs > 0), just like they are today.

colesbury commented 1 week ago

I've updated https://github.com/python/cpython/issues/117376.