Skip to content
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

Vector caching. #3292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Vector caching. #3292

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 73 additions & 2 deletions src_c/math.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ static PyTypeObject pgVectorIter_Type;

#define pgVector2_Check(x) (PyType_IsSubtype(Py_TYPE(x), &pgVector2_Type))
#define pgVector3_Check(x) (PyType_IsSubtype(Py_TYPE(x), &pgVector3_Type))
#define pgVector2_CheckExact(x) (Py_TYPE(x) == &pgVector2_Type)
#define pgVector3_CheckExact(x) (Py_TYPE(x) == &pgVector3_Type)
#define pgVector_Check(x) (pgVector2_Check(x) || pgVector3_Check(x))
#define pgVector_CheckExact(x) \
(pgVector2_CheckExact(x) || pgVector3_CheckExact(x))
#define vector_elementwiseproxy_Check(x) \
(Py_TYPE(x) == &pgVectorElementwiseProxy_Type)
#define _vector_subtype_new(x) \
Expand All @@ -111,6 +115,31 @@ typedef struct {
PyObject_HEAD pgVector *vec;
} vector_elementwiseproxy;

#define VECTOR_CACHE_SIZE 100
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this is 100, I mean, I would expect it to be bigger.
To be fair, these numbers are hard to get right though. I would expect numbers like this to be backed up by empirical test data. A simple test app that uses vectors extensively would be a good start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true. I don't have experience in that.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if you could write a simple benchmark that demonstrates the exact performance enhancements this approach can give.


static pgVector *vec2_cache[VECTOR_CACHE_SIZE] = {};
static int vec2_cache_num = 0;
static pgVector *vec3_cache[VECTOR_CACHE_SIZE] = {};
static int vec3_cache_num = 0;

#define _VEC_CACHE_ADD(x, v) \
{ \
if (vec##x##_cache_num < VECTOR_CACHE_SIZE) { \
Py_INCREF(v); \
vec##x##_cache[vec##x##_cache_num] = v; \
vec##x##_cache_num++; \
} \
}

#define _VEC_CACHE_POP(x, v) \
{ \
if (vec##x##_cache_num > 0) { \
vec##x##_cache_num--; \
(v) = vec##x##_cache[vec##x##_cache_num]; \
vec##x##_cache[vec##x##_cache_num] = NULL; \
} \
}

/* further forward declarations */
/* math functions */
static PyObject *
Expand Down Expand Up @@ -628,9 +657,33 @@ pgVector_NEW(Py_ssize_t dim)
static void
vector_dealloc(pgVector *self)
{
if (pgVector_CheckExact(self) &&
PyObject_CallFinalizerFromDealloc((PyObject *)self) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I see no mention of this function in the C API docs. What does it do exactly?

Also, I think there is a possibility of introducing a memory leak here. What happens when the cache is full? I would expect vectors to be dealloc'd normally in that case, but it looks like those vectors will be leaked in this implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see no mention of this function in the C API docs. What does it do exactly?

python/cpython#78090

Also, I think there is a possibility of introducing a memory leak here. What happens when the cache is full?

PyObject_CallFinalizerFromDealloc returns non-zero value only when object is resurrected [1] (the ref-count is increased while finalizing, which only happens when cache is not full - _VEC_CACHE_ADD).

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit skeptical on using undocumented Python C API, but for now I think it's okay

return;
}
Py_TYPE(self)->tp_free((PyObject *)self);
}

static void
vector_finalize(PyObject *self)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this logic kept in a separate finalize function, is there a particular reason it should not be a part of dealloc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used for tp_finalize, not tp_dealloc in PyTypeObject struct. "Resurrecting" an objects seems to be allowed only in tp_finalize and not tp_dealloc. I have to explicitly call PyObject_CallFinalizerFromDealloc due to quirkiness of default CPython deallocator (for static types - heap types and python types don't have this issue), which just doesn't call it (PyObject_CallFinalizerFromDealloc handles the logic behind tp_finalize).

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, yeah that makes sense

{
if (!pgVector_CheckExact(self))
return;

pgVector *vec = (pgVector *)self;

switch (vec->dim) {
case 2:
_VEC_CACHE_ADD(2, vec)
break;
case 3:
_VEC_CACHE_ADD(3, vec)
break;
default:
break;
}
}

/**********************************************
* Generic vector PyNumber emulation routines
**********************************************/
Expand Down Expand Up @@ -2190,7 +2243,15 @@ vector___round__(pgVector *self, PyObject *args)
static PyObject *
vector2_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
pgVector *vec = (pgVector *)type->tp_alloc(type, 0);
pgVector *vec = NULL;

if (type == &pgVector2_Type) {
_VEC_CACHE_POP(2, vec)
}

if (vec != NULL)
return (PyObject *)vec;
vec = (pgVector *)type->tp_alloc(type, 0);

if (vec != NULL) {
vec->dim = 2;
Expand Down Expand Up @@ -2596,6 +2657,7 @@ static PyTypeObject pgVector2_Type = {
PyVarObject_HEAD_INIT(NULL, 0).tp_name = "pygame.math.Vector2",
.tp_basicsize = sizeof(pgVector),
.tp_dealloc = (destructor)vector_dealloc,
.tp_finalize = (destructor)vector_finalize,
.tp_repr = (reprfunc)vector_repr,
.tp_as_number = &vector_as_number,
.tp_as_sequence = &vector_as_sequence,
Expand All @@ -2621,7 +2683,15 @@ static PyTypeObject pgVector2_Type = {
static PyObject *
vector3_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
pgVector *vec = (pgVector *)type->tp_alloc(type, 0);
pgVector *vec = NULL;

if (type == &pgVector3_Type) {
_VEC_CACHE_POP(3, vec)
}

if (vec != NULL)
return (PyObject *)vec;
vec = (pgVector *)type->tp_alloc(type, 0);

if (vec != NULL) {
vec->dim = 3;
Expand Down Expand Up @@ -3489,6 +3559,7 @@ static PyTypeObject pgVector3_Type = {
PyVarObject_HEAD_INIT(NULL, 0).tp_name = "pygame.math.Vector3",
.tp_basicsize = sizeof(pgVector),
.tp_dealloc = (destructor)vector_dealloc,
.tp_finalize = (destructor)vector_finalize,
.tp_repr = (reprfunc)vector_repr,
.tp_as_number = &vector_as_number,
.tp_as_sequence = &vector_as_sequence,
Expand Down
Loading