Preparation for finalizer code, refactoring some simple_destructor logic. #666

rudi-c · 2015-07-02T18:47:25Z

Mostly moving simple_destructor functions into tp_dealloc instead of their own field. Replace the field with a boolean to indicate whether the tp_dealloc has the safety properties of a simple_destructor. Assign simple_destructor to more builtin types.

Depends on gcfinalizers2 PR.

rudi-c · 2015-07-02T19:42:26Z

              pyston (calibration)                      :    1.0s baseline: 1.0 (+0.7%)
              pyston django_template.py                 :    4.5s baseline: 4.5 (-0.2%)
              pyston pyxl_bench.py                      :    3.7s baseline: 3.6 (+1.2%)
              pyston sqlalchemy_imperative2.py          :    5.1s baseline: 5.1 (+0.6%)
              pyston django_migrate.py                  :    1.7s baseline: 1.7 (-0.3%)
              pyston virtualenv_bench.py                :    5.2s baseline: 5.2 (+0.3%)

rudi-c · 2015-07-02T19:43:05Z

Oh nice I have PR 666 😈

toshok · 2015-07-13T18:29:57Z

from_cpython/Modules/itertoolsmodule.c

@@ -4080,6 +4080,30 @@ static PyMethodDef module_methods[] = {
    {NULL,              NULL}           /* sentinel */
 };

+PyTypeObject* Itertool_SafeDealloc_Types[] = {
+    // &combinations_type,
+    // &cwr_type,


are these commented out because they're not actually safe?

I copied the static array from PyTypeObject *typelist[] that has all the types in the file, then commented out those that I wasn't sure was safe yeah.

definitely add that to the comment (maybe with a TODO to take a further look). confusing otherwise.

rudi-c · 2015-07-14T19:26:52Z

Updated -- ready for review again. That threading_local test still fails but that's fine right?

kmod · 2015-07-15T21:50:51Z

lgtm (pending #662) though I think it'd be nice to address proxy_to_tp_clear. I kicked travis-ci so it's retesting and should hopefully not hit that threading_local issue a second time :)

rudi-c · 2015-07-16T00:41:37Z

Ok, I replaced that to use the default tp_dealloc (which does the same thing as tp_clear for weakrefs in addition to a few CPython GC-things that we don't run).

rudi-c · 2015-07-16T19:59:11Z

Actually no that's not true. tp_dealloc and tp_clear for proxy are different and we can't use tp_dealloc, I'm reverting that.

toshok · 2015-07-16T23:09:08Z

what does "address proxy_to_tp_clear" mean? missing context :)

rudi-c · 2015-07-16T23:19:32Z

Replacing tp_dealloc = proxy_to_tp_clear with just the original tp_dealloc. I don't think that works though.

toshok · 2015-07-16T23:26:33Z

hm, I think it will if tp_free is set to default_free.

weakref_dealloc is just:

    PyObject_GC_UnTrack(self);
    clear_weakref((PyWeakReference *) self);
    Py_TYPE(self)->tp_free(self);

and proxy_dealloc is just:

    if (self->wr_callback != NULL)
        PyObject_GC_UnTrack((PyObject *)self);
    clear_weakref(self);
    PyObject_GC_Del(self);

gc_clear only does the clear_weakref call.

The wildcards are PyObject_GC_UnTrack (that's either a macro that does nothing or a function in gcmodule.c that definitely won't work for us.) and PyObject_GC_Del.

and PyObject_GC_Del appears to call (through a few macro expansions) PyObject_Free which does gc_compat_free, which definitely won't work for this. okay, I'm convinced.

rudi-c · 2015-07-16T23:28:06Z

That one is fine, I tried setting tp_free. The problem is:

static void                                       
proxy_dealloc(PyWeakReference *self)              
{                                                 
    if (self->wr_callback != NULL)                
        PyObject_GC_UnTrack((PyObject *)self);    
    clear_weakref(self);                          
    PyObject_GC_Del(self);                        
}

toshok · 2015-07-16T23:35:36Z

src/runtime/types.cpp

+static void setTypeGCProxy(BoxedClass* cls) {
+    cls->tp_alloc = PystonType_GenericAlloc;
+    cls->gc_visit = proxy_to_tp_traverse;
+    cls->tp_dealloc = proxy_to_tp_clear;


might be good to add a comment here "we can't use the normal tp_dealloc here, since proxy_dealloc explicitly frees self but doesn't go through tp_free, etc..." so we don't forget and ask the questions again :)

Replace the function pointer to the simple_destructor with a boolean indicating that the tp_dealloc function is safe to call whenever the simple_destructor used to be instead. A few additional classes are also specified to have a safe_tp_dealloc. For exceptions, use a hack where we look for the creation of exception classes and store them in a list so we can set their destructor at the same time as other classes.

kmod · 2015-07-16T23:58:34Z

I think we're a bit off-track on the proxy_to_tp_clear issue -- tp_clear is the wrong attribute for us to be relying on, and tp_dealloc is the right one. There's nothing particularly special about the proxy_dealloc method, and while it's ok for us to make some restrictions on the c extension code we'll support, it seems like this is a pretty bad workaround for a case that we need to support anyway.

toshok · 2015-07-17T00:25:19Z

nod. I mean we can just change the proxy tp_dealloc to do the same tp_free invocation that weakref_dealloc does (and set tp_free to default_free). The only reason I didn't feel too strongly either way about this is that this particular method is already very specialized and only used for the weakref types.

is there any reason we couldn't just change PyObject_GC_Del(o) to go through tp_free?

toshok · 2015-07-17T17:55:58Z

@rudi-c okay, so steps forward from here are, I think:

verify that PyObject_GC_Del is actually hitting gc_compat_free (I can't remember if that's dependent on #defines we aren't specifying.. maybe it's a no-op for us?)
depending on 1, either leave PyObject_GC_Del in, comment it out, or change to the tp_free call like weakref_dealloc uses (the latter will take some additional work since you'll have to change alloc sites to be proper). Then likely get rid of proxy_to_tp_clear + setTypeGCProxy, and switch the weakref types to using the same method as the other builtins.
check other extension tp_deallocs (at least in our builtin extension modules) to make sure they aren't calling PyObject_GC_Del and are otherwise safe.
add an issue for auditing the tp_deallocs across all the source we need to run to make sure they aren't doing anything funny.

sound good?

Preparation for finalizer code, refactoring some simple_destructor logic.

toshok · 2015-07-17T21:19:25Z

👍

rudi-c force-pushed the gcfinalizers3 branch from 7b02d64 to fa9e971 Compare July 2, 2015 18:47

rudi-c force-pushed the gcfinalizers3 branch 5 times, most recently from f0695a8 to ad9c048 Compare July 6, 2015 21:33

rudi-c mentioned this pull request Jul 6, 2015

Call finalizers during garbage collection with ordering #672

Merged

rudi-c force-pushed the gcfinalizers3 branch 2 times, most recently from 808b807 to 252f8d6 Compare July 8, 2015 20:57

toshok reviewed Jul 13, 2015
View reviewed changes

rudi-c force-pushed the gcfinalizers3 branch 3 times, most recently from 7d96a26 to 2b595e7 Compare July 14, 2015 17:19

kmod assigned toshok Jul 15, 2015

rudi-c force-pushed the gcfinalizers3 branch 2 times, most recently from f95dbf3 to 2f0bf7d Compare July 16, 2015 00:40

rudi-c force-pushed the gcfinalizers3 branch from 2f0bf7d to 9ff75d8 Compare July 16, 2015 19:58

rudi-c force-pushed the gcfinalizers3 branch 2 times, most recently from 030a20e to a5cf65a Compare July 16, 2015 22:58

toshok reviewed Jul 16, 2015
View reviewed changes

rudi-c force-pushed the gcfinalizers3 branch from a5cf65a to 22b388e Compare July 16, 2015 23:38

Use weakref's original tp_dealloc.

fb2515a

rudi-c force-pushed the gcfinalizers3 branch from 169163b to fb2515a Compare July 17, 2015 18:53

toshok added a commit that referenced this pull request Jul 17, 2015

Merge pull request #666 from rudi-c/gcfinalizers3

e648044

Preparation for finalizer code, refactoring some simple_destructor logic.

toshok merged commit e648044 into pyston:master Jul 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparation for finalizer code, refactoring some simple_destructor logic. #666

Preparation for finalizer code, refactoring some simple_destructor logic. #666

rudi-c commented Jul 2, 2015

rudi-c commented Jul 2, 2015

rudi-c commented Jul 2, 2015

toshok Jul 13, 2015

rudi-c Jul 13, 2015

toshok Jul 13, 2015

rudi-c commented Jul 14, 2015

kmod commented Jul 15, 2015

rudi-c commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok Jul 16, 2015

kmod commented Jul 16, 2015

toshok commented Jul 17, 2015

toshok commented Jul 17, 2015

toshok commented Jul 17, 2015

Preparation for finalizer code, refactoring some simple_destructor logic. #666

Preparation for finalizer code, refactoring some simple_destructor logic. #666

Conversation

rudi-c commented Jul 2, 2015

rudi-c commented Jul 2, 2015

rudi-c commented Jul 2, 2015

toshok Jul 13, 2015

Choose a reason for hiding this comment

rudi-c Jul 13, 2015

Choose a reason for hiding this comment

toshok Jul 13, 2015

Choose a reason for hiding this comment

rudi-c commented Jul 14, 2015

kmod commented Jul 15, 2015

rudi-c commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok commented Jul 16, 2015

rudi-c commented Jul 16, 2015

toshok Jul 16, 2015

Choose a reason for hiding this comment

kmod commented Jul 16, 2015

toshok commented Jul 17, 2015

toshok commented Jul 17, 2015

toshok commented Jul 17, 2015