[Patches] Sightly clean-up cycle-gc patch
Greg Stein
gstein@lyra.org
Wed, 26 Apr 2000 05:38:11 -0700 (PDT)
On Tue, 25 Apr 2000, Charles G Waldman wrote:
>...
> --- Python-cvs/Include/object.h Fri Mar 24 22:32:16 2000
> +++ Python-gc/Include/object.h Sat Apr 8 01:38:25 2000
> @@ -145,6 +145,10 @@
> typedef int (*getsegcountproc) Py_PROTO((PyObject *, int *));
> typedef int (*getcharbufferproc) Py_PROTO((PyObject *, int, const char **));
> typedef int (*objobjproc) Py_PROTO((PyObject *, PyObject *));
> +#ifdef WITH_CYCLE_GC
> +typedef int (*visitproc) Py_PROTO((PyObject *, void *));
> +typedef int (*recurseproc) Py_PROTO((PyObject *, visitproc, void *));
> +#endif
>
> typedef struct {
> binaryfunc nb_add;
> @@ -243,9 +247,18 @@
>
> char *tp_doc; /* Documentation string */
>
> - /* More spares */
> +#ifdef WITH_CYCLE_GC
> + /* call function for all accessible objects */
> + recurseproc tp_recurse;
> +
> + /* delete references to contained objects */
> + inquiry tp_clear;
> +#else
> long tp_xxx5;
> long tp_xxx6;
> +#endif
Just rename those spares. I don't see that we could truly use them for
something else -- if somebody defined WITH_CYCLE_GC, then there would be a
conflict.
This also implies the typedefs further above would not be protected by the
macro.
>...
> --- Python-cvs/Include/objimpl.h Thu Mar 2 17:02:23 2000
> +++ Python-gc/Include/objimpl.h Sat Apr 8 03:18:22 2000
> @@ -38,8 +38,42 @@
> /*
> Additional macros for modules that implement new object types.
> You must first include "object.h".
> +*/
> +
> +#ifdef WITH_CYCLE_GC
> +
> +/* Structure *prefixed* to container objects participating in GC */
> +typedef struct _gcinfo {
> + struct _gcinfo *gc_next;
> + struct _gcinfo *gc_prev;
> + int gc_refs;
> +} PyGCInfo;
> +
> +/* Test if a type has GC info */
> +#define PY_TYPEISGC(t) PyType_HasFeature((t), Py_TPFLAGS_HAVE_GCINFO)
> +
> +/* Test if an object has GC info */
> +#define PY_ISGC(o) PY_TYPEISGC((o)->ob_type)
> +
> +/* Get an object's GC info -- NULL if the object has none */
> +#define PY_GCINFO(o) (PY_ISGC(o) ? ((PyGCInfo *)(o)-1) : NULL)
> +
> +/* Unsafe version of PY_GCINFO() -- only call if PY_ISGC(p) is true */
> +#define PY_GCINFO_UNSAFE(o) ((PyGCInfo *)(o)-1)
> +
> +/* Get the object given the PyGCInfo */
> +#define PY_GCOBJ(g) ((PyObject *)((g)+1))
IMO, if it doesn't change the Python functionality, then don't protect
this stuff with WITH_CYCLE_GC.
These macros should also be named Py_* rather than PY_*. I'm not away of
anything using the PY_ prefix.
> -PyObject_NEW(type, typeobj) allocates memory for a new object of the given
> +/* Add the object into the container set */
> +extern DL_IMPORT(void) PyGC_Insert Py_PROTO((PyObject *));
> +
> +/* Remove the object from the container set */
> +extern DL_IMPORT(void) PyGC_Remove Py_PROTO((PyObject *));
These functions probably need to be protected. Seems like I recall a
linker that will look for them if an "extern" is found anywhere in the
program (even if no call is made to it).
>...
> --- Python-cvs/Modules/newmodule.c Tue Feb 29 14:55:09 2000
> +++ Python-gc/Modules/newmodule.c Sat Apr 8 02:13:26 2000
> @@ -49,13 +49,16 @@
> &PyClass_Type, &klass,
> &PyDict_Type, &dict))
> return NULL;
> - inst = PyObject_NEW(PyInstanceObject, &PyInstance_Type);
> + inst = PyObject_New(PyInstanceObject, &PyInstance_Type);
> if (inst == NULL)
> return NULL;
> Py_INCREF(klass);
> Py_INCREF(dict);
> inst->in_class = (PyClassObject *)klass;
> inst->in_dict = dict;
> +#ifdef WITH_CYCLE_GC
> + PyGC_Insert((PyObject *)inst);
> +#endif
This would be MUCH cleaner if PyGC_Insert() evaluated to a no-op in the
non-GC case.
>...
> +/*** list functions (should probably be macros, stupid compilers) ***/
> +
> +static void
> +LIST_INIT(PyGCInfo *list)
But since they are not macros, the upper case is "wierd." I'd recommend
making them lower case, like the rest of Python functions.
>...
> @@ -498,11 +528,14 @@
> PyObject *error_type, *error_value, *error_traceback;
> PyObject *del;
> static PyObject *delstr;
> + extern long _Py_RefTotal;
> +#ifdef WITH_CYCLE_GC
> + PyGC_Remove((PyObject *)inst);
> +#endif
Same on PyGC_Remove() ... make it a no-op macro when WITH_CYCLE_GC is not
defined.
>...
> - im = PyObject_NEW(PyMethodObject, &PyMethod_Type);
> + im = PyObject_New(PyMethodObject, &PyMethod_Type);
Can these changes be broken out into a separate patch? I'm guessing that
this can be done *regardless* of the GC work, and that it would be an
improvement to things. If that is the case, then let's see a separate
patch for JUST this and get it applied ASAP. Could reduce Vladimir's patch
size, too.
I'm a big fan of avoiding comingling of patch concepts...
>...
> @@ -489,7 +495,7 @@
> }
> }
> PyMem_XDEL(mp->ma_table);
> - PyMem_DEL(mp);
> + PyObject_Del((PyObject *)mp);
> Py_TRASHCAN_SAFE_END(mp)
> }
Same logic applies here. Let's see this change in a basic patch that the
GC patches can THEN build upon.
By getting small pieces, each individually reviewable, each individually
applicable, then we have a larger hope of a successful review of these
patches. As it is, I'm only doing a cursory review over this stuff because
the dumb thing is so big...
>...
> --- Python-cvs/Objects/object.c Tue Mar 28 08:11:09 2000
> +++ Python-gc/Objects/object.c Sat Apr 8 02:10:06 2000
> @@ -107,20 +107,12 @@
> }
> #endif
>
> -#ifndef MS_COREDLL
> -PyObject *
> -_PyObject_New(tp)
> - PyTypeObject *tp;
> -#else
> +
> PyObject *
> -_PyObject_New(tp,op)
> +_PyObject_FromType(tp,op)
> PyTypeObject *tp;
> PyObject *op;
> -#endif
> {
> -#ifndef MS_COREDLL
> - PyObject *op = (PyObject *) malloc(tp->tp_basicsize);
> -#endif
> if (op == NULL)
> return PyErr_NoMemory();
> op->ob_type = tp;
> @@ -128,29 +120,78 @@
> return op;
> }
>
> -#ifndef MS_COREDLL
> -PyVarObject *
> -_PyObject_NewVar(tp, size)
> - PyTypeObject *tp;
> - int size;
> -#else
> PyVarObject *
> -_PyObject_NewVar(tp, size, op)
> +_PyObject_VarFromType(tp, size, op)
> PyTypeObject *tp;
> int size;
> PyVarObject *op;
> -#endif
> {
> -#ifndef MS_COREDLL
> - PyVarObject *op = (PyVarObject *)
> - malloc(tp->tp_basicsize + size * tp->tp_itemsize);
> -#endif
Umm.... what is this stuff with removing MS_COREDLL? Is that right? Is
this part of the PyObject_New thing? Should it go into the separate patch?
>...
> #if MAXSAVESIZE > 0
> if (size == 0) {
> free_tuples[0] = op;
> - ++num_free_tuples[0];
Woah! Did you start with an old version of this file? Maybe you should do
a "cvs update" before generating your diff, to make sure that CVS changes
are integrated into your local source.
>...
> #if MAXSAVESIZE > 0
> if (newsize == 0 && free_tuples[0]) {
> - num_free_tuples[0]--;
> sv = free_tuples[0];
> sv->ob_size = 0;
Another one here.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/