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

Fidget-Spinner · 2024-04-30T21:09:40Z

This PR sets up tagged pointers for CPython.

The general idea is to create a separate struct _PyStackRef for everything on the evaluation stack to store the bits. This forces the C compiler to warn us if we try to cast things or pull things out of the struct directly.

Only for free threading: We tag the low bit if something is deferred - that means we skip incref and decref operations on it. This behavior may change in the future if Mark's plans to defer all objects in the interpreter loop pans out.

This implies a strict stack reference discipline is required. ALL incref and decref operations on stackrefs must use the stackref variants. It is unsafe to untag something then do normal incref/decref ops on it.

The new incref and decref variants are called dup and close. They mimic a "handle" API operating on these stackrefs.

Please read Include/internal/pycore_stackref.h for more information!

Issue: Set up tagged pointers in the evaluation stack #117139

gvanrossum · 2024-04-30T21:13:08Z

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 · 2024-04-30T21:18:26Z

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 · 2024-04-30T21:57:34Z

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.

colesbury

I left some comments below, mostly minor. I'll take a closer look at bytecodes.c and the generated cases next.

Python/gc_free_threading.c

Include/internal/pycore_call.h

Include/internal/pycore_ceval.h

Include/internal/pycore_dict.h

Include/internal/pycore_frame.h

Objects/dictobject.c

Objects/frameobject.c

Python/bytecodes.c

Fidget-Spinner · 2024-06-26T09:50:42Z

@markshannon the last legitimately failing buildbot now passes https://buildbot.python.org/all/#/builders/1364/builds/150 !

It seems ASAN actually caught a real bug there, where vectorcall implicitly assumes the -1 entry of a stack is available for writing self to.

Fidget-Spinner · 2024-06-26T15:55:34Z

I'm going to merge this in an hour. We can keep iterating on it if needed after.

All addressed, or can be done in follow-up PRs.

markshannon · 2024-06-26T17:23:02Z

It seems ASAN actually caught a real bug there, where vectorcall implicitly assumes the -1 entry of a stack is available for writing self to.

It isn't implicit it depends on the flags pass https://peps.python.org/pep-0590/. Take care to set the flags to suit your buffer size.
The buildbot run you linked to was a success. Where was the failure?

Fidget-Spinner · 2024-06-26T17:28:58Z

A few comments, but nothing substantial

Yeah I recall the flag, but I just blanket put +1 extra space for everything for simplicity's sake. The failure is here https://buildbot.python.org/all/#/builders/1364/builds/143.

Fidget-Spinner · 2024-06-26T17:31:33Z

Nevermind, I'll change it so it checks for the flags now. Thanks PY_VECTORCALL_ARGUMENTS_OFFSET!

Fidget-Spinner · 2024-06-26T18:33:45Z

!buildbot x86-64 MacOS Intel ASAN NoGIL PR

bedevere-bot · 2024-06-26T18:33:48Z

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit ff41e0c 🤖

The command will test the builders whose names match following regular expression: x86-64 MacOS Intel ASAN NoGIL PR

The builders matched are:

x86-64 MacOS Intel ASAN NoGIL PR

…` arguments in configuration command after #118450 (#121083) Signed-off-by: Manjusaka <me@manjusaka.me> Co-authored-by: Ken Jin <kenjin4096@gmail.com>

…18450) This PR sets up tagged pointers for CPython. The general idea is to create a separate struct _PyStackRef for everything on the evaluation stack to store the bits. This forces the C compiler to warn us if we try to cast things or pull things out of the struct directly. Only for free threading: We tag the low bit if something is deferred - that means we skip incref and decref operations on it. This behavior may change in the future if Mark's plans to defer all objects in the interpreter loop pans out. This implies a strict stack reference discipline is required. ALL incref and decref operations on stackrefs must use the stackref variants. It is unsafe to untag something then do normal incref/decref ops on it. The new incref and decref variants are called dup and close. They mimic a "handle" API operating on these stackrefs. Please read Include/internal/pycore_stackref.h for more information! --------- Co-authored-by: Mark Shannon <9448417+markshannon@users.noreply.github.com>

…ystats` arguments in configuration command after python#118450 (python#121083) Signed-off-by: Manjusaka <me@manjusaka.me> Co-authored-by: Ken Jin <kenjin4096@gmail.com>

…18450) This PR sets up tagged pointers for CPython. The general idea is to create a separate struct _PyStackRef for everything on the evaluation stack to store the bits. This forces the C compiler to warn us if we try to cast things or pull things out of the struct directly. Only for free threading: We tag the low bit if something is deferred - that means we skip incref and decref operations on it. This behavior may change in the future if Mark's plans to defer all objects in the interpreter loop pans out. This implies a strict stack reference discipline is required. ALL incref and decref operations on stackrefs must use the stackref variants. It is unsafe to untag something then do normal incref/decref ops on it. The new incref and decref variants are called dup and close. They mimic a "handle" API operating on these stackrefs. Please read Include/internal/pycore_stackref.h for more information! --------- Co-authored-by: Mark Shannon <9448417+markshannon@users.noreply.github.com>

…ystats` arguments in configuration command after python#118450 (python#121083) Signed-off-by: Manjusaka <me@manjusaka.me> Co-authored-by: Ken Jin <kenjin4096@gmail.com>

…18450) This PR sets up tagged pointers for CPython. The general idea is to create a separate struct _PyStackRef for everything on the evaluation stack to store the bits. This forces the C compiler to warn us if we try to cast things or pull things out of the struct directly. Only for free threading: We tag the low bit if something is deferred - that means we skip incref and decref operations on it. This behavior may change in the future if Mark's plans to defer all objects in the interpreter loop pans out. This implies a strict stack reference discipline is required. ALL incref and decref operations on stackrefs must use the stackref variants. It is unsafe to untag something then do normal incref/decref ops on it. The new incref and decref variants are called dup and close. They mimic a "handle" API operating on these stackrefs. Please read Include/internal/pycore_stackref.h for more information! --------- Co-authored-by: Mark Shannon <9448417+markshannon@users.noreply.github.com>

…ystats` arguments in configuration command after python#118450 (python#121083) Signed-off-by: Manjusaka <me@manjusaka.me> Co-authored-by: Ken Jin <kenjin4096@gmail.com>

Fidget-Spinner added 6 commits May 1, 2024 04:54

merge from upstream

a6275f1

remove test tags

7e7627f

add test for immortal

00dde8e

Merge remote-tracking branch 'upstream/main' into stackref_all

eadc4fc

minor cleanups

64e2800

Create 2024-05-01-05-09-16.gh-issue-117139.t41w_D.rst

bf6b7e4

bedevere-app bot mentioned this pull request Apr 30, 2024

Set up tagged pointers in the evaluation stack #117139

Closed

Fidget-Spinner marked this pull request as ready for review April 30, 2024 21:09

Fidget-Spinner requested review from a team, gvanrossum, kumaraditya303, brettcannon, ericsnowcurrently, ncoghlan, warsaw, markshannon, methane and brandtbucher as code owners April 30, 2024 21:09

bedevere-app bot added the awaiting core review label Apr 30, 2024

Fidget-Spinner requested a review from colesbury April 30, 2024 21:10

remove bad assert

865ff8d

Fidget-Spinner added the topic-free-threading label May 1, 2024

rename tagged -> stackref

b1777c4

colesbury reviewed May 1, 2024

View reviewed changes

Fidget-Spinner added 3 commits May 3, 2024 04:02

trivial fixes first

50cfefa

fix anti-patterns (use deferred everywhere)

39e9057

Delete 2024-03-26-20-31-34.gh-issue-117139.eELvoZ.rst

1bb3c51

Merge remote-tracking branch 'upstream/main' into stackref_all

50c92d0

Fidget-Spinner requested a review from markshannon June 26, 2024 14:32

Fidget-Spinner added 4 commits June 27, 2024 01:42

Fix bug in malloc case

ce77fe4

Use immortal

193e216

Remove borrowed load in counter_optimize

30f7a09

silence warning

ff41e0c

colesbury approved these changes Jun 26, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Jun 26, 2024

Fidget-Spinner merged commit 22b0de2 into python:main Jun 26, 2024
66 of 67 checks passed

bedevere-app bot removed the awaiting merge label Jun 26, 2024

Fidget-Spinner deleted the stackref_all branch June 26, 2024 19:10

Zheaoli mentioned this pull request Jun 27, 2024

CPython build failed with --enable-pystats after #118450 #121082

Closed

This was referenced Jul 2, 2024

Stackrefs (#118450) introduced a large performance regression on Windows #121263

Closed

gh-121263: Macro-ify most stackref functions for MSVC #121270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fidget-Spinner commented Apr 30, 2024 •

edited

Loading

gvanrossum commented Apr 30, 2024

Fidget-Spinner commented Apr 30, 2024

colesbury commented Apr 30, 2024

colesbury left a comment

Fidget-Spinner commented Jun 26, 2024 •

edited

Loading

Fidget-Spinner commented Jun 26, 2024

markshannon commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

bedevere-bot commented Jun 26, 2024

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

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

Conversation

Fidget-Spinner commented Apr 30, 2024 • edited Loading

gvanrossum commented Apr 30, 2024

Fidget-Spinner commented Apr 30, 2024

colesbury commented Apr 30, 2024

colesbury left a comment

Choose a reason for hiding this comment

Fidget-Spinner commented Jun 26, 2024 • edited Loading

Fidget-Spinner commented Jun 26, 2024

markshannon commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

Fidget-Spinner commented Jun 26, 2024

bedevere-bot commented Jun 26, 2024

Fidget-Spinner commented Apr 30, 2024 •

edited

Loading

Fidget-Spinner commented Jun 26, 2024 •

edited

Loading