Skip to content

GH-119866: Spill stack pointer when making "escaping" calls. #119875

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.

Sign up for GitHub

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

Closed
markshannon wants to merge 7 commits into from
Closed

GH-119866: Spill stack pointer when making "escaping" calls. #119875

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd94999
Experiment to measure maximum objects and space deferred without addi…
markshannon May 30, 2024
e12729e
Strip out instrumentation
markshannon May 30, 2024
f748355
Convert stacktop to pointer
markshannon May 31, 2024
0a50860
Remove deferred RC stats
markshannon May 31, 2024
c2775e0
Remove OBJECT_STAT_ADD
markshannon May 31, 2024
2bd1339
Restore whitespace
markshannon May 31, 2024
792c553
Add many more SAVE_SP/LOAD_SP pairs
markshannon Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove deferred RC stats
  • Loading branch information
@markshannon
markshannon committed May 31, 2024
commit 0a5086080c3b09a020e6ff552ff15add57b1541f
1 change: 1 addition & 0 deletions Include/cpython/pystate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ struct _ts {
PyObject *previous_executor;

uint64_t dict_global_version;
int sp_cached; /* Only used in debug builds */

@mdboom mdboom May 31, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If only used in debug builds, should this be inside #ifdef Py_DEBUG?

@brandtbucher brandtbucher May 31, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think debug builds have the same ABI compatibility as release builds these days. I know we like to pretend that PyThreadState is opaque, but this seems like it might case some nasty bugs if we do this.


};

Expand Down
19 changes: 0 additions & 19 deletions Include/cpython/pystats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,8 @@ typedef struct _object_stats {
uint64_t type_cache_dunder_hits;
uint64_t type_cache_dunder_misses;
uint64_t type_cache_collisions;
uint64_t max_deferred_space;
uint64_t max_deferred_objects;
/* Temporary value used during GC */
uint64_t object_visits;
uint64_t deferred_space;
uint64_t deferred_objects;
} ObjectStats;

typedef struct _gc_stats {
Expand Down Expand Up @@ -171,18 +167,3 @@ PyAPI_DATA(PyStats*) _Py_stats;
# define _Py_INCREF_STAT_INC() do { if (_Py_stats) _Py_stats->object_stats.increfs++; } while (0)
# define _Py_DECREF_STAT_INC() do { if (_Py_stats) _Py_stats->object_stats.decrefs++; } while (0)
#endif


#define UPDATE_DEFERRED_STATS() \
do { \
if (_Py_stats) { \
if (_Py_stats->object_stats.deferred_space > _Py_stats->object_stats.max_deferred_space) { \
_Py_stats->object_stats.max_deferred_space = _Py_stats->object_stats.deferred_space; \
} \
if (_Py_stats->object_stats.deferred_objects > _Py_stats->object_stats.max_deferred_objects) { \
_Py_stats->object_stats.max_deferred_objects = _Py_stats->object_stats.deferred_objects; \
} \
_Py_stats->object_stats.deferred_space = 0; \
_Py_stats->object_stats.deferred_objects = 0; \
} \
} while (0)
1 change: 0 additions & 1 deletion Include/internal/pycore_code.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,6 @@ PyAPI_FUNC(PyObject*) _Py_GetSpecializationStats(void);
#define OPT_ERROR_IN_OPCODE(opname) ((void)0)
#define OPT_HIST(length, name) ((void)0)
#define RARE_EVENT_STAT_INC(name) ((void)0)
#define UPDATE_DEFERRED_STATS() ((void)0)
#endif // !Py_STATS

// Utility functions for reading/writing 32/64-bit values in the inline caches.
Expand Down
6 changes: 6 additions & 0 deletions Include/internal/pycore_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ _PyFrame_GetLocalsArray(_PyInterpreterFrame *frame)
static inline PyObject**
_PyFrame_GetStackPointer(_PyInterpreterFrame *frame)
{
#ifndef Py_DEBUG

@dfremont dfremont Jun 5, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this (and the corresponding line in the next function) be #ifdef Py_DEBUG? I don't think the assertion added to _Py_Dealloc will ever see an updated value of sp_cached otherwise.

@markshannon markshannon Jun 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks.

PyThreadState_GET()->sp_cached++;
#endif
assert(frame->stackpointer != NULL);
PyObject **sp = frame->stackpointer;
frame->stackpointer = NULL;
Expand All @@ -174,6 +177,9 @@ _PyFrame_GetStackPointer(_PyInterpreterFrame *frame)
static inline void
_PyFrame_SetStackPointer(_PyInterpreterFrame *frame, PyObject **stack_pointer)
{
#ifndef Py_DEBUG
PyThreadState_GET()->sp_cached--;
#endif
assert(frame->stackpointer == NULL);
frame->stackpointer = stack_pointer;
}
Expand Down
1 change: 0 additions & 1 deletion Include/pystats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ extern "C" {
#else
# define _Py_INCREF_STAT_INC() ((void)0)
# define _Py_DECREF_STAT_INC() ((void)0)
# define UPDATE_DEFERRED_STATS() ((void)0)
#endif // !Py_STATS

#ifdef __cplusplus
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_sys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,7 +1561,7 @@ class C(object): pass
def func():
return sys._getframe()
x = func()
check(x, size('3Pi2cP7P2ic??2P'))
check(x, size('3Pi2cP9Phc2P'))
# function
def func(): pass
check(func, size('16Pi'))
Expand All @@ -1578,7 +1578,7 @@ def bar(cls):
check(bar, size('PP'))
# generator
def get_gen(): yield 1
check(get_gen(), size('PP4P4c7P2ic??2P'))
check(get_gen(), size('PP4P4cP9PhcP'))
# iterator
check(iter('abc'), size('lP'))
# callable-iterator
Expand Down
1 change: 1 addition & 0 deletions Objects/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2829,6 +2829,7 @@ _Py_Dealloc(PyObject *op)
destructor dealloc = type->tp_dealloc;
#ifdef Py_DEBUG
PyThreadState *tstate = _PyThreadState_GET();
assert(tstate->sp_cached <= 1);
PyObject *old_exc = tstate != NULL ? tstate->current_exception : NULL;
// Keep the old exception type alive to prevent undefined behavior
// on (tstate->curexc_type != old_exc_type) below
Expand Down
1 change: 0 additions & 1 deletion Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ dummy_func(
uintptr_t eval_breaker = _Py_atomic_load_uintptr_relaxed(&tstate->eval_breaker);
uintptr_t version = FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(_PyFrame_GetCode(frame)->_co_instrumentation_version);
assert((version & _PY_EVAL_EVENTS_MASK) == 0);
UPDATE_DEFERRED_STATS();
DEOPT_IF(eval_breaker != version);
}

Expand Down
3 changes: 1 addition & 2 deletions Python/ceval_macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,7 @@ do { \
if (err != 0) { \
GOTO_ERROR(error); \
} \
} \
UPDATE_DEFERRED_STATS();
}


/* Tuple access macros */
Expand Down
1 change: 0 additions & 1 deletion Python/executor_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,7 @@ init_threadstate(_PyThreadStateImpl *_tstate,
tstate->what_event = -1;
tstate->previous_executor = NULL;
tstate->dict_global_version = 0;
tstate->sp_cached = 0;

tstate->delete_later = NULL;

Expand Down
4 changes: 0 additions & 4 deletions Python/specialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,6 @@ print_object_stats(FILE *out, ObjectStats *stats)
fprintf(out, "Object method cache collisions: %" PRIu64 "\n", stats->type_cache_collisions);
fprintf(out, "Object method cache dunder hits: %" PRIu64 "\n", stats->type_cache_dunder_hits);
fprintf(out, "Object method cache dunder misses: %" PRIu64 "\n", stats->type_cache_dunder_misses);
fprintf(out, "Object max deferred objects: %" PRIu64 "\n", stats->max_deferred_objects);
fprintf(out, "Object max deferred space: %" PRIu64 "\n", stats->max_deferred_space);
}

static void
Expand Down Expand Up @@ -321,8 +319,6 @@ void
_Py_StatsOn(void)
{
_Py_stats = &_Py_stats_struct;
_Py_stats->object_stats.deferred_objects = 0;
_Py_stats->object_stats.deferred_space = 0;
}

void
Expand Down