Skip _PyType_Lookup for monomorphic LOAD/STORE_ATTR opcodes

Every generic attribute lookup (ie no overridden __getattribute__) starts by hitting _PyType_Lookup on the type and then maybe hitting the object dict if it exists.  If we know the type through feedback, we can 1) cache the result of _PyType_Lookup (the descriptor) and 2) skip the dict lookup if the dict doesn't exist.

This is implemented by digging out the code from PyObject_Generic[G|S]etAttr and inlining it with extra constant parameters that are fixed at compilation time.  LLVM then propagates those constants through the inlined body and does the Right Thing.

2to3:
22.120000 -> 21.800000: 1.47% faster

django:
Min: 0.594583 -> 0.582381: 2.10% faster
Avg: 0.604536 -> 0.588952: 2.65% faster
Significant (t=17.944955, a=0.95)
Stddev: 0.00518 -> 0.00330: 57.04% smaller

rietveld:
Min: 0.392147 -> 0.381066: 2.91% faster
Avg: 0.399033 -> 0.389779: 2.37% faster
Significant (t=5.930455, a=0.95)
Stddev: 0.00808 -> 0.00752: 7.39% smaller

slowpickle:
Min: 0.389578 -> 0.351653: 10.78% faster
Avg: 0.395050 -> 0.352043: 12.22% faster
Significant (t=95.702589, a=0.95)
Stddev: 0.00317 -> 0.00018: 1631.34% smaller

slowspitfire:
Min: 0.346205 -> 0.337817: 2.48% faster
Avg: 0.347899 -> 0.339405: 2.50% faster
Significant (t=8.062652, a=0.95)
Stddev: 0.00531 -> 0.00523: 1.61% smaller

slowunpickle:
Min: 0.193255 -> 0.172468: 12.05% faster
Avg: 0.195854 -> 0.172629: 13.45% faster
Significant (t=132.029606, a=0.95)
Stddev: 0.00124 -> 0.00011: 1057.79% smaller

[Reid Kleckner, 2009-11-23 23:27:33]

PTAL

[Jeffrey Yasskin, 2009-11-25 02:32:53]

http://codereview.appspot.com/157130/diff/1013/37
File Include/object.h (right):

http://codereview.appspot.com/157130/diff/1013/37#newcode402
Include/object.h:402: /* Code objects listening for modifications.  Added in
Unladen.  */
Could you mention the type of PyObject this points to?

http://codereview.appspot.com/157130/diff/1013/38
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/1013/38#newcode2982
Lib/test/test_llvm.py:2982: # Check that the code bails correctly by passing an
argument of another
Could you also check that a different argument of the same type doesn't bail?

http://codereview.appspot.com/157130/diff/1013/38#newcode2991
Lib/test/test_llvm.py:2991: # Check that even though it bails, it bails
successfully.
s/it bails successfully/it gets the right answer/ ?

http://codereview.appspot.com/157130/diff/1013/38#newcode2995
Lib/test/test_llvm.py:2995: def test_load_attr_fast_no_descr(self):
Could you spell out "descriptor"?
The previous test also had no descriptor. Perhaps
"test_load_attr_fast_new_descriptor_invalidates"?

http://codereview.appspot.com/157130/diff/1013/38#newcode3014
Lib/test/test_llvm.py:3014: C.foo = property(new_get_foo)
Comment that this overrides c.foo. (You check this below, but you should say it
explicitly.)

http://codereview.appspot.com/157130/diff/1013/38#newcode3024
Lib/test/test_llvm.py:3024: __slots__ = ('foo',)
Does it make sense to also test a class whose __slots__ contains '__dict__'?
http://docs.python.org/reference/datamodel.html#__slots__

http://codereview.appspot.com/157130/diff/1013/38#newcode3060
Lib/test/test_llvm.py:3060: self.assertEqual(0, get_foo(c))
I think you can skip checking it for correctness while it's spinning. Do check
before and after the loop.  test_llvm is already slow enough that it's nice to
avoid 11000 calls where we can.

http://codereview.appspot.com/157130/diff/1013/38#newcode3063
Lib/test/test_llvm.py:3063: # Check that invalidating the code by assigning a
descriptor works.
I think you should also try invalidating it by changing foo to a non-descriptor.

http://codereview.appspot.com/157130/diff/1013/38#newcode3076
Lib/test/test_llvm.py:3076: return 1
What happens if the descriptor replaces itself on the class, but only after
compilation finishes?

http://codereview.appspot.com/157130/diff/1013/38#newcode3094
Lib/test/test_llvm.py:3094: # We can't do C.foo, because that calls __get__
Wow. Descriptors make me sad.

http://codereview.appspot.com/157130/diff/1013/38#newcode3102
Lib/test/test_llvm.py:3102: class C(object):
Can we have different names here? Or a function to do the main part of the test
to ensure it's the same between compiled and interpreted code?

http://codereview.appspot.com/157130/diff/1013/38#newcode3163
Lib/test/test_llvm.py:3163: self.assertTrue(get_foo.__code__.__use_llvm__)
Could you comment why we don't have to invalidate the machine code when the
class grows a data descriptor?

http://codereview.appspot.com/157130/diff/1013/38#newcode3226
Lib/test/test_llvm.py:3226: # Make a descriptor class and then mutate the class
so that it's a
I think this comment is out of date.

http://codereview.appspot.com/157130/diff/1013/38#newcode3282
Lib/test/test_llvm.py:3282: def test_store_attr_fast_invalidates(self):
I think you need most of the load_attr tests replicated for store_attr.

http://codereview.appspot.com/157130/diff/1013/41
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/1013/41#newcode95
Objects/typeobject.c:95: raw = type->tp_code_listeners;
"raw"?

http://codereview.appspot.com/157130/diff/1013/41#newcode4110
Objects/typeobject.c:4110: _PyType_AddCodeListener(PyTypeObject *type, PyObject
*code)
Hm, instead of duplicating this code and add_subclass, can you extract the
common bits?

http://codereview.appspot.com/157130/diff/1013/42
File Python/eval.cc (right):

http://codereview.appspot.com/157130/diff/1013/42#newcode75
Python/eval.cc:75: fprintf(stderr, "\n%zd code objects deemed hot\n",
this->hot_code_.size());
80 columns

http://codereview.appspot.com/157130/diff/1013/42#newcode75
Python/eval.cc:75: fprintf(stderr, "\n%zd code objects deemed hot\n",
this->hot_code_.size());
Why do we have tabs in a new file?

http://codereview.appspot.com/157130/diff/1013/42#newcode75
Python/eval.cc:75: fprintf(stderr, "\n%zd code objects deemed hot\n",
this->hot_code_.size());
s/printf(/fprintf(stderr, /g should be a separate change.

http://codereview.appspot.com/157130/diff/1013/43
File Python/llvm_compile.cc (right):

http://codereview.appspot.com/157130/diff/1013/43#newcode410
Python/llvm_compile.cc:410:
fbuilder.function()->setLinkage(llvm::GlobalValue::ExternalLinkage);
Don't set the function to external linkage if we aren't guaranteed to hand
ownership to a PyObject. That'll cause the function to be leaked.

... It might make sense to move the setLinkage() call into _LlvmFunction_New().

http://codereview.appspot.com/157130/diff/1013/44
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
Probably is_load should be an enum so call sites are readable.

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
s/index/names_index/ ?

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
Do we have a convention for the "can I optimize this" function's name? "Check"
doesn't seem quite specific enough.

http://codereview.appspot.com/157130/diff/1013/44#newcode1357
Python/llvm_fbuilder.cc:1357: PyObject **name_out)
Please document whether these return borrowed or owned references.

http://codereview.appspot.com/157130/diff/1013/44#newcode1367
Python/llvm_fbuilder.cc:1367: name = PyUnicode_AsEncodedString(name, NULL,
NULL);
After this, we don't know whether name owns its reference, so there's
necessarily a leak.

Also, I didn't see any tests for unicode attribute names.

http://codereview.appspot.com/157130/diff/1013/44#newcode1383
Python/llvm_fbuilder.cc:1383: ACCESS_ATTR_INC_STATS(no_opt_no_data);
"Overflowed" is a polymorphic error, not a no_data error.

http://codereview.appspot.com/157130/diff/1013/44#newcode1421
Python/llvm_fbuilder.cc:1421: void *descr_access,
You're just using descr_access as a bool. Why not pass it as one?

http://codereview.appspot.com/157130/diff/1013/44#newcode1424
Python/llvm_fbuilder.cc:1424: Value *type_v,
Could you comment where each of these arguments are supposed to come from? Are
they compile-time things or runtime things? Which type is this?

http://codereview.appspot.com/157130/diff/1013/44#newcode1433
Python/llvm_fbuilder.cc:1433: // will be invalidated before the type object is
freed.  Therefore we don't
I didn't see a test for a freed type object.

http://codereview.appspot.com/157130/diff/1013/44#newcode1433
Python/llvm_fbuilder.cc:1433: // will be invalidated before the type object is
freed.  Therefore we don't
LOAD_GLOBAL explicitly checks code.co_use_llvm and bails if it's 0. I don't see
that here.

http://codereview.appspot.com/157130/diff/1013/44#newcode1447
Python/llvm_fbuilder.cc:1447:
this->builder_.CreateLoad(ObjectTy::ob_type(this->builder_, descr_v));
80 columns

http://codereview.appspot.com/157130/diff/1013/44#newcode1484
Python/llvm_fbuilder.cc:1484: // Perform the common object attribute access
checks and emit the common
CheckAttributeAccess doesn't emit any code.

http://codereview.appspot.com/157130/diff/1013/44#newcode1492
Python/llvm_fbuilder.cc:1492: // Lookup all the data from the type that we want
to cache.
Describe how you're using NULL.

http://codereview.appspot.com/157130/diff/1013/44#newcode1509
Python/llvm_fbuilder.cc:1509: Value *type_v =
this->EmbedPointer<PyTypeObject*>(type);
It's odd that type is not the type of obj...

http://codereview.appspot.com/157130/diff/1013/44#newcode1509
Python/llvm_fbuilder.cc:1509: Value *type_v =
this->EmbedPointer<PyTypeObject*>(type);
How are we holding a reference to type? Is that from the feedback object?

http://codereview.appspot.com/157130/diff/1013/44#newcode1581
Python/llvm_fbuilder.cc:1581: // Perform the common object attribute access
checks and emit the common
Same comment here.

http://codereview.appspot.com/157130/diff/1013/44#newcode1590
Python/llvm_fbuilder.cc:1590: PyObject *descr = _PyType_Lookup(type, name);
This code seems to be common with LOAD_ATTR_fast. It would be nice to share it.

http://codereview.appspot.com/157130/diff/1013/44#newcode4051
Python/llvm_fbuilder.cc:4051: LlvmFunctionBuilder::EmbedPointer(void *ptr)
In CALL_FUNCTION_fast, there's another use of this concept, with a TODO. Could
you replace all the similar IntToPtrs with calls to this function and move the
TODO here?

http://codereview.appspot.com/157130/diff/1013/46
File Python/llvm_inline_functions.c (right):

http://codereview.appspot.com/157130/diff/1013/46#newcode173
Python/llvm_inline_functions.c:173: /* Keep this in sync with
_PyObject_GetDictPtr.  We need it inlined in order
Please add the matching "keep this in sync" comments to object.c.

http://codereview.appspot.com/157130/diff/1013/47
File Util/PyTypeBuilder.h (right):

http://codereview.appspot.com/157130/diff/1013/47#newcode545
Util/PyTypeBuilder.h:545: // We happen to have two functions with these types,
so we must define type
Yikes.

[Reid Kleckner, 2009-12-04 20:49:20]

http://codereview.appspot.com/157130/diff/1013/37
File Include/object.h (right):

http://codereview.appspot.com/157130/diff/1013/37#newcode402
Include/object.h:402: /* Code objects listening for modifications.  Added in
Unladen.  */
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Could you mention the type of PyObject this points to?

Done.

http://codereview.appspot.com/157130/diff/1013/38
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/1013/38#newcode2982
Lib/test/test_llvm.py:2982: # Check that the code bails correctly by passing an
argument of another
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Could you also check that a different argument of the same type doesn't bail?

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode2991
Lib/test/test_llvm.py:2991: # Check that even though it bails, it bails
successfully.
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> s/it bails successfully/it gets the right answer/ ?

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode2995
Lib/test/test_llvm.py:2995: def test_load_attr_fast_no_descr(self):
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Could you spell out "descriptor"?
> The previous test also had no descriptor. Perhaps
> "test_load_attr_fast_new_descriptor_invalidates"?

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode3014
Lib/test/test_llvm.py:3014: C.foo = property(new_get_foo)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Comment that this overrides c.foo. (You check this below, but you should say
it
> explicitly.)

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode3024
Lib/test/test_llvm.py:3024: __slots__ = ('foo',)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Does it make sense to also test a class whose __slots__ contains '__dict__'?
> http://docs.python.org/reference/datamodel.html#__slots__

I don't think so, since it doesn't significantly change the code paths used in
the attribute lookup optimization.  Adding a dict slot sets tp_dictoffset, which
I handle the same way as I do any other object.

http://codereview.appspot.com/157130/diff/1013/38#newcode3060
Lib/test/test_llvm.py:3060: self.assertEqual(0, get_foo(c))
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> I think you can skip checking it for correctness while it's spinning. Do check
> before and after the loop.  test_llvm is already slow enough that it's nice to
> avoid 11000 calls where we can.

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode3063
Lib/test/test_llvm.py:3063: # Check that invalidating the code by assigning a
descriptor works.
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> I think you should also try invalidating it by changing foo to a
non-descriptor.

That will just mutate the type and invalidate the code, as done in this test. 
The really interesting test is modifying the type of the descriptor object, so
that the code is *not* invalidated, but the descriptor is mutated from a
descriptor to a non-descriptor.

http://codereview.appspot.com/157130/diff/1013/38#newcode3076
Lib/test/test_llvm.py:3076: return 1
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> What happens if the descriptor replaces itself on the class, but only after
> compilation finishes?

I added a test for this.

http://codereview.appspot.com/157130/diff/1013/38#newcode3102
Lib/test/test_llvm.py:3102: class C(object):
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Can we have different names here? Or a function to do the main part of the
test
> to ensure it's the same between compiled and interpreted code?

Sure, I abstracted the common code that mutates the __class__ attribute and
asserts that it works correctly.

http://codereview.appspot.com/157130/diff/1013/38#newcode3163
Lib/test/test_llvm.py:3163: self.assertTrue(get_foo.__code__.__use_llvm__)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Could you comment why we don't have to invalidate the machine code when the
> class grows a data descriptor?

It's not that we don't have to, but the current implementation does not, because
it is difficult.  We would either have to build a callback mechanism for object
class mutation, or we could invalidate the code when we descriptor type guard
fails, since unless the descriptor type is eventually changed back, it will
always bail.  IMO it's just not worth the effort, since that guard will never
fail in a sane program anyway.

http://codereview.appspot.com/157130/diff/1013/38#newcode3226
Lib/test/test_llvm.py:3226: # Make a descriptor class and then mutate the class
so that it's a
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> I think this comment is out of date.

Done.

http://codereview.appspot.com/157130/diff/1013/38#newcode3282
Lib/test/test_llvm.py:3282: def test_store_attr_fast_invalidates(self):
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> I think you need most of the load_attr tests replicated for store_attr.

Are you sure?  I've changed the code so that they share all of the code paths
that deal with guards and invalidation.

http://codereview.appspot.com/157130/diff/1013/41
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/1013/41#newcode95
Objects/typeobject.c:95: raw = type->tp_code_listeners;
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> "raw"?

Made more descriptive.

http://codereview.appspot.com/157130/diff/1013/41#newcode4110
Objects/typeobject.c:4110: _PyType_AddCodeListener(PyTypeObject *type, PyObject
*code)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Hm, instead of duplicating this code and add_subclass, can you extract the
> common bits?

Done.  I've been thinking we should refactor this and the PyDict_Watcher stuff
into an object that stores two values: a callback for modification notification,
and the PyObject to notify.

http://codereview.appspot.com/157130/diff/1013/42
File Python/eval.cc (right):

http://codereview.appspot.com/157130/diff/1013/42#newcode75
Python/eval.cc:75: fprintf(stderr, "\n%zd code objects deemed hot\n",
this->hot_code_.size());
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Why do we have tabs in a new file?

It's eval.cc, silly, it still uses Python C style.  ;)

http://codereview.appspot.com/157130/diff/1013/42#newcode75
Python/eval.cc:75: fprintf(stderr, "\n%zd code objects deemed hot\n",
this->hot_code_.size());
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> s/printf(/fprintf(stderr, /g should be a separate change.

Reverted.  I needed it to separate the output from regular stdout when running
the benchmarks under perf.py.

http://codereview.appspot.com/157130/diff/1013/43
File Python/llvm_compile.cc (right):

http://codereview.appspot.com/157130/diff/1013/43#newcode410
Python/llvm_compile.cc:410:
fbuilder.function()->setLinkage(llvm::GlobalValue::ExternalLinkage);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Don't set the function to external linkage if we aren't guaranteed to hand
> ownership to a PyObject. That'll cause the function to be leaked.
> 
> ... It might make sense to move the setLinkage() call into
_LlvmFunction_New().

Done.

http://codereview.appspot.com/157130/diff/1013/44
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Probably is_load should be an enum so call sites are readable.

I tried to use the opcodes (LOAD_ATTR, STORE_ATTR), but the methods of the same
name shadow those names.

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> s/index/names_index/ ?

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1355
Python/llvm_fbuilder.cc:1355: LlvmFunctionBuilder::CheckAttributeAccess(bool
is_load, int index,
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Do we have a convention for the "can I optimize this" function's name? "Check"
> doesn't seem quite specific enough.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1357
Python/llvm_fbuilder.cc:1357: PyObject **name_out)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Please document whether these return borrowed or owned references.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1367
Python/llvm_fbuilder.cc:1367: name = PyUnicode_AsEncodedString(name, NULL,
NULL);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> After this, we don't know whether name owns its reference, so there's
> necessarily a leak.
> 
> Also, I didn't see any tests for unicode attribute names.

I deleted this code, since according to the instrumentation, it's dead.

http://codereview.appspot.com/157130/diff/1013/44#newcode1383
Python/llvm_fbuilder.cc:1383: ACCESS_ATTR_INC_STATS(no_opt_no_data);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> "Overflowed" is a polymorphic error, not a no_data error.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1421
Python/llvm_fbuilder.cc:1421: void *descr_access,
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> You're just using descr_access as a bool. Why not pass it as one?

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1424
Python/llvm_fbuilder.cc:1424: Value *type_v,
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Could you comment where each of these arguments are supposed to come from? Are
> they compile-time things or runtime things? Which type is this?

I've moved those values into a class that does this caching lookup.  Everything
determined statically comes off of the class, and everything else is a
parameter.

http://codereview.appspot.com/157130/diff/1013/44#newcode1433
Python/llvm_fbuilder.cc:1433: // will be invalidated before the type object is
freed.  Therefore we don't
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> I didn't see a test for a freed type object.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1433
Python/llvm_fbuilder.cc:1433: // will be invalidated before the type object is
freed.  Therefore we don't
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> LOAD_GLOBAL explicitly checks code.co_use_llvm and bails if it's 0. I don't
see
> that here.

Done.  I also wrote a test that exercises it.

http://codereview.appspot.com/157130/diff/1013/44#newcode1447
Python/llvm_fbuilder.cc:1447:
this->builder_.CreateLoad(ObjectTy::ob_type(this->builder_, descr_v));
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> 80 columns

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1484
Python/llvm_fbuilder.cc:1484: // Perform the common object attribute access
checks and emit the common
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> CheckAttributeAccess doesn't emit any code.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1492
Python/llvm_fbuilder.cc:1492: // Lookup all the data from the type that we want
to cache.
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Describe how you're using NULL.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1509
Python/llvm_fbuilder.cc:1509: Value *type_v =
this->EmbedPointer<PyTypeObject*>(type);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> It's odd that type is not the type of obj...

Changed to guard_type_v.

http://codereview.appspot.com/157130/diff/1013/44#newcode1509
Python/llvm_fbuilder.cc:1509: Value *type_v =
this->EmbedPointer<PyTypeObject*>(type);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> How are we holding a reference to type? Is that from the feedback object?

I've tried to more clearly document how the code object holds no reference to
the type object.  During compilation, we use the reference from the feedback. 
After compilation, if the type object is freed, it calls PyType_Modified, which
invalidates our code object.

http://codereview.appspot.com/157130/diff/1013/44#newcode1581
Python/llvm_fbuilder.cc:1581: // Perform the common object attribute access
checks and emit the common
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Same comment here.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode1590
Python/llvm_fbuilder.cc:1590: PyObject *descr = _PyType_Lookup(type, name);
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> This code seems to be common with LOAD_ATTR_fast. It would be nice to share
it.

Done.

http://codereview.appspot.com/157130/diff/1013/44#newcode4051
Python/llvm_fbuilder.cc:4051: LlvmFunctionBuilder::EmbedPointer(void *ptr)
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> In CALL_FUNCTION_fast, there's another use of this concept, with a TODO. Could
> you replace all the similar IntToPtrs with calls to this function and move the
> TODO here?

Done.  The TODO was only relevant to embedding function pointers for equality
testing, though, so I left it there since it is the only place that tests
function pointer equality.

http://codereview.appspot.com/157130/diff/1013/46
File Python/llvm_inline_functions.c (right):

http://codereview.appspot.com/157130/diff/1013/46#newcode173
Python/llvm_inline_functions.c:173: /* Keep this in sync with
_PyObject_GetDictPtr.  We need it inlined in order
On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> Please add the matching "keep this in sync" comments to object.c.

Done.

[Jeffrey Yasskin, 2009-12-05 01:38:42]

Looks pretty good. Thanks!

http://codereview.appspot.com/157130/diff/1013/38
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/1013/38#newcode3063
Lib/test/test_llvm.py:3063: # Check that invalidating the code by assigning a
descriptor works.
On 2009/12/04 20:49:20, Reid Kleckner wrote:
> On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> > I think you should also try invalidating it by changing foo to a
> non-descriptor.
> 
> That will just mutate the type and invalidate the code, as done in this test. 
> The really interesting test is modifying the type of the descriptor object, so
> that the code is *not* invalidated, but the descriptor is mutated from a
> descriptor to a non-descriptor.

Ah, I see. Could you comment that here and below? That is, something about how
this covers changing C.foo in any way (including del?), and below that it avoids
the check on changing C.foo.

http://codereview.appspot.com/157130/diff/1013/38#newcode3076
Lib/test/test_llvm.py:3076: return 1
On 2009/12/04 20:49:20, Reid Kleckner wrote:
> On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> > What happens if the descriptor replaces itself on the class, but only after
> > compilation finishes?
> 
> I added a test for this.

Which test is that? I'm not just worried about the class being changed inside
the same function, but actually inside the same operation that we're guarding.
That kind of recursion has caused bugs in the past.

http://codereview.appspot.com/157130/diff/1013/38#newcode3163
Lib/test/test_llvm.py:3163: self.assertTrue(get_foo.__code__.__use_llvm__)
On 2009/12/04 20:49:20, Reid Kleckner wrote:
> On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> > Could you comment why we don't have to invalidate the machine code when the
> > class grows a data descriptor?
> 
> It's not that we don't have to, but the current implementation does not,
because
> it is difficult.  We would either have to build a callback mechanism for
object
> class mutation, or we could invalidate the code when we descriptor type guard
> fails, since unless the descriptor type is eventually changed back, it will
> always bail.  IMO it's just not worth the effort, since that guard will never
> fail in a sane program anyway.

'k

http://codereview.appspot.com/157130/diff/1013/38#newcode3282
Lib/test/test_llvm.py:3282: def test_store_attr_fast_invalidates(self):
On 2009/12/04 20:49:20, Reid Kleckner wrote:
> On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> > I think you need most of the load_attr tests replicated for store_attr.
> 
> Are you sure?  I've changed the code so that they share all of the code paths
> that deal with guards and invalidation.

Never mind. :)

http://codereview.appspot.com/157130/diff/1013/41
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/1013/41#newcode4110
Objects/typeobject.c:4110: _PyType_AddCodeListener(PyTypeObject *type, PyObject
*code)
On 2009/12/04 20:49:20, Reid Kleckner wrote:
> On 2009/11/25 02:32:53, Jeffrey Yasskin wrote:
> > Hm, instead of duplicating this code and add_subclass, can you extract the
> > common bits?
> 
> Done.  I've been thinking we should refactor this and the PyDict_Watcher stuff
> into an object that stores two values: a callback for modification
notification,
> and the PyObject to notify.

Maybe. Although the dictwatcher doesn't use weakrefs, and we don't want to add
too many fields to the objects we attach these to.

http://codereview.appspot.com/157130/diff/1013/44
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/157130/diff/1013/44#newcode1258
Python/llvm_fbuilder.cc:1258: Value *global = ConstantExpr::getIntToPtr(
Wait a second, why is this using IntToPtr instead of ConstantMirror? Bah, not
your fault anyway.

http://codereview.appspot.com/157130/diff/3001/4002
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/3001/4002#newcode3111
Lib/test/test_llvm.py:3111: sys.setbailerror(False)
Is the code actually going to bail inside the get-hot loop? I was confused
reading this the first time thinking that get_foo() might have been compiled
before the loop, and the loop was going to re-compile it. If it's not going to
bail until afterwards, probably move this to after the loop? Or assert that
__use_llvm__ is false before the loop?

http://codereview.appspot.com/157130/diff/3001/4002#newcode3282
Lib/test/test_llvm.py:3282: import gc, weakref
I believe you can import these at the top of the file. If you can, you should.

http://codereview.appspot.com/157130/diff/3001/4002#newcode3300
Lib/test/test_llvm.py:3300: # This function will likely bail because it takes a
cold branch
Ah, isn't testing a JIT lovely?

http://codereview.appspot.com/157130/diff/3001/4002#newcode3304
Lib/test/test_llvm.py:3304: # a cold branch, so we that off while we evaluate
the conditional.
gr: so we that off

http://codereview.appspot.com/157130/diff/3001/4002#newcode3325
Lib/test/test_llvm.py:3325: self.assertRaises(RuntimeError, get_foo, c)
With bailerror==False, shouldn't this return 2 instead of raising?

Ah, I see what's happening: maybe_invalidate sets bailerror back to True instead
of back to its initial value. I'd prefer the test for the right value instead of
for bailing, I think.

http://codereview.appspot.com/157130/diff/3001/4005
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/3001/4005#newcode109
Objects/typeobject.c:109: type->tp_code_listeners = NULL;
You should clear this before decref'ing its contents, in case (somehow) python
code gets called from the destructor. Probably null it out just after reading it
into listeners.

http://codereview.appspot.com/157130/diff/3001/4006
File PCbuild/pythoncore.vcproj (left):

http://codereview.appspot.com/157130/diff/3001/4006#oldcode2024
PCbuild/pythoncore.vcproj:2024: RelativePath="..\Util\Stats.cc"
Did you mean to change this?

http://codereview.appspot.com/157130/diff/3001/4008
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/157130/diff/3001/4008#newcode1389
Python/llvm_fbuilder.cc:1389: bool descr_has_accessor = (accessor.descr_get_ !=
NULL);
This may be too picky, but the AttributeAccessor knows whether it's going to be
a get or a set. Maybe unify the descr_get and descr_set fields and avoid passing
it in at all?

http://codereview.appspot.com/157130/diff/3001/4008#newcode1470
Python/llvm_fbuilder.cc:1470: // Fill in the bail bb.
You could push the bail block into GuardAttributeAccess.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1552
Python/llvm_fbuilder.cc:1552: bool overridden = false;
true is the safer default. Alternately, set it in the else for opt mode.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1594
Python/llvm_fbuilder.cc:1594: this->descr_get_ = NULL;
Probably null out all the fields in the constructor.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1651
Python/llvm_fbuilder.cc:1651: // If the descriptor has a getter method, we need
to guard on the
Why is the getter special if this is a STORE_ATTR?

http://codereview.appspot.com/157130/diff/3001/4009
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/157130/diff/3001/4009#newcode560
Python/llvm_fbuilder.h:560: class AttributeAccessor {
Feel free to put this in the .cc file if that's the only place it's used. It
should probably be in an anonymous namespace there, if you move it.

Doing that would make dependencies a bit easier to track since you wouldn't be
able to look data up through the fbuilder. On the other hand, the fbuilder's
utility functions would be inaccessible. Bah.

http://codereview.appspot.com/157130/diff/3001/4009#newcode563
Python/llvm_fbuilder.h:563: PyObject *name, AttrAccessKind kind)
Align this line with the '('.

http://codereview.appspot.com/157130/diff/3001/4009#newcode563
Python/llvm_fbuilder.h:563: PyObject *name, AttrAccessKind kind)
What name? The comments here should say what the code looks like that this gets
triggered on, and how the pieces of that code get passed to this object, and
where they live once they're here.

http://codereview.appspot.com/157130/diff/3001/4009#newcode590
Python/llvm_fbuilder.h:590: // if there is no attribute for that name.
"that name" is imprecise.

http://codereview.appspot.com/157130/diff/3001/4009#newcode594
Python/llvm_fbuilder.h:594: PyTypeObject *guard_descr_type_;
What's "guard" refer to? Is this field == descr_->ob_type?

http://codereview.appspot.com/157130/diff/3001/4009#newcode601
Python/llvm_fbuilder.h:601: // llvm::Value versions of the above data.
These fields' names should be a simple transform of the PyObject field they
mirror, or they should have a comment saying what expression the value
represents. Are they computed with ConstantMirror, IntToPtr, or some other
method?

http://codereview.appspot.com/157130/diff/3001/4009#newcode622
Python/llvm_fbuilder.h:622: friend class AttributeAccessor;
This isn't necessary since AttributeAccessor is a member, and all members have
access to the class's fields.

http://codereview.appspot.com/157130/diff/3001/4011
File Util/ConstantMirror.cc (right):

http://codereview.appspot.com/157130/diff/3001/4011#newcode325
Util/ConstantMirror.cc:325: // You can't put a const Constant into another
Constant,
This comment applied to the const_cast that's now elsewhere.

http://codereview.appspot.com/157130/diff/3001/4011#newcode444
Util/ConstantMirror.cc:444: assert(0 && "Invalid arity");
This should be a "Py_FatalError;return NULL", I think, or maybe a "return
PyErr_BadInternalCall()". In opt mode, we shouldn't just fall off the end of a
function that returns a value.

http://codereview.appspot.com/157130/diff/3001/4012
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/157130/diff/3001/4012#newcode15
Util/ConstantMirror.h:15: template <typename T> class PyTypeBuilder;
You need to #include the PyTypeBuilder header instead of forward-declaring it.
You must be getting it somehow anyway or the ::get call wouldn't work, but you
should make it explicit.

[Reid Kleckner, 2009-12-05 07:06:29]

http://codereview.appspot.com/157130/diff/3001/4002
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/3001/4002#newcode3111
Lib/test/test_llvm.py:3111: sys.setbailerror(False)
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Is the code actually going to bail inside the get-hot loop? I was confused
> reading this the first time thinking that get_foo() might have been compiled
> before the loop, and the loop was going to re-compile it. If it's not going to
> bail until afterwards, probably move this to after the loop? Or assert that
> __use_llvm__ is false before the loop?

I've moved all of the setbailerrors after the get-hot loops.  Does that clarify
it?

http://codereview.appspot.com/157130/diff/3001/4002#newcode3282
Lib/test/test_llvm.py:3282: import gc, weakref
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> I believe you can import these at the top of the file. If you can, you should.

Done.

http://codereview.appspot.com/157130/diff/3001/4002#newcode3304
Lib/test/test_llvm.py:3304: # a cold branch, so we that off while we evaluate
the conditional.
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> gr: so we that off

Done.

http://codereview.appspot.com/157130/diff/3001/4002#newcode3325
Lib/test/test_llvm.py:3325: self.assertRaises(RuntimeError, get_foo, c)
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> With bailerror==False, shouldn't this return 2 instead of raising?
> 
> Ah, I see what's happening: maybe_invalidate sets bailerror back to True
instead
> of back to its initial value. I'd prefer the test for the right value instead
of
> for bailing, I think.

OK, I've removed the other setbailerrors since they are now unnecessary.

http://codereview.appspot.com/157130/diff/3001/4005
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/3001/4005#newcode109
Objects/typeobject.c:109: type->tp_code_listeners = NULL;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> You should clear this before decref'ing its contents, in case (somehow) python
> code gets called from the destructor. Probably null it out just after reading
it
> into listeners.

Done.

http://codereview.appspot.com/157130/diff/3001/4006
File PCbuild/pythoncore.vcproj (left):

http://codereview.appspot.com/157130/diff/3001/4006#oldcode2024
PCbuild/pythoncore.vcproj:2024: RelativePath="..\Util\Stats.cc"
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Did you mean to change this?

It shouldn't be here.  I'm using Mercurial named branches, and I essentially did
"hg diff skip-type-lookup:trunk" and trunk was more recent than the last
revision of trunk that I had merged with.  It'll be gone in my next diff.

http://codereview.appspot.com/157130/diff/3001/4008
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/157130/diff/3001/4008#newcode1389
Python/llvm_fbuilder.cc:1389: bool descr_has_accessor = (accessor.descr_get_ !=
NULL);
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> This may be too picky, but the AttributeAccessor knows whether it's going to
be
> a get or a set. Maybe unify the descr_get and descr_set fields and avoid
passing
> it in at all?

Done.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1470
Python/llvm_fbuilder.cc:1470: // Fill in the bail bb.
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> You could push the bail block into GuardAttributeAccess.

Done.  They used to have different bail blocks, but then I waited to pop value
until after the guards.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1552
Python/llvm_fbuilder.cc:1552: bool overridden = false;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> true is the safer default. Alternately, set it in the else for opt mode.

Done.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1594
Python/llvm_fbuilder.cc:1594: this->descr_get_ = NULL;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Probably null out all the fields in the constructor.

Done.

http://codereview.appspot.com/157130/diff/3001/4008#newcode1651
Python/llvm_fbuilder.cc:1651: // If the descriptor has a getter method, we need
to guard on the
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Why is the getter special if this is a STORE_ATTR?

Actually, I don't know what I was thinking when I wrote this.  Whenever there is
a descriptor, we need to guard on its type and subscribe for type modifications,
in case it suddenly grows tp_descr_get and tp_descr_set methods.  This version
of the code only guards on a descriptor changing or losing methods, not growing
methods.

I've fixed the bug and added a test case.

http://codereview.appspot.com/157130/diff/3001/4009
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/157130/diff/3001/4009#newcode560
Python/llvm_fbuilder.h:560: class AttributeAccessor {
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Feel free to put this in the .cc file if that's the only place it's used. It
> should probably be in an anonymous namespace there, if you move it.
> 
> Doing that would make dependencies a bit easier to track since you wouldn't be
> able to look data up through the fbuilder. On the other hand, the fbuilder's
> utility functions would be inaccessible. Bah.

Yeah, I think it has to stay in the header in order to mark it as a friend class
of the fbuilder so it can generate IR.

http://codereview.appspot.com/157130/diff/3001/4009#newcode563
Python/llvm_fbuilder.h:563: PyObject *name, AttrAccessKind kind)
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> Align this line with the '('.

Done.

http://codereview.appspot.com/157130/diff/3001/4009#newcode563
Python/llvm_fbuilder.h:563: PyObject *name, AttrAccessKind kind)
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> What name? The comments here should say what the code looks like that this
gets
> triggered on, and how the pieces of that code get passed to this object, and
> where they live once they're here.

Done.

http://codereview.appspot.com/157130/diff/3001/4009#newcode590
Python/llvm_fbuilder.h:590: // if there is no attribute for that name.
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> "that name" is imprecise.

Done.

http://codereview.appspot.com/157130/diff/3001/4009#newcode594
Python/llvm_fbuilder.h:594: PyTypeObject *guard_descr_type_;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> What's "guard" refer to? Is this field == descr_->ob_type?

Commented.

http://codereview.appspot.com/157130/diff/3001/4009#newcode601
Python/llvm_fbuilder.h:601: // llvm::Value versions of the above data.
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> These fields' names should be a simple transform of the PyObject field they
> mirror, or they should have a comment saying what expression the value
> represents. Are they computed with ConstantMirror, IntToPtr, or some other
> method?

I'm not sure what you mean, they are all simple transforms of the other fields
with a "v_" postfix.  They're all created via EmbedPointer or ConstantInt::get.

http://codereview.appspot.com/157130/diff/3001/4009#newcode622
Python/llvm_fbuilder.h:622: friend class AttributeAccessor;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> This isn't necessary since AttributeAccessor is a member, and all members have
> access to the class's fields.

Oh, fixed.

http://codereview.appspot.com/157130/diff/3001/4011
File Util/ConstantMirror.cc (right):

http://codereview.appspot.com/157130/diff/3001/4011#newcode325
Util/ConstantMirror.cc:325: // You can't put a const Constant into another
Constant,
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> This comment applied to the const_cast that's now elsewhere.

Done.

http://codereview.appspot.com/157130/diff/3001/4011#newcode444
Util/ConstantMirror.cc:444: assert(0 && "Invalid arity");
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> This should be a "Py_FatalError;return NULL", I think, or maybe a "return
> PyErr_BadInternalCall()". In opt mode, we shouldn't just fall off the end of a
> function that returns a value.

Done.

http://codereview.appspot.com/157130/diff/3001/4012
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/157130/diff/3001/4012#newcode15
Util/ConstantMirror.h:15: template <typename T> class PyTypeBuilder;
On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> You need to #include the PyTypeBuilder header instead of forward-declaring it.
> You must be getting it somehow anyway or the ::get call wouldn't work, but you
> should make it explicit.

Done.  I had to break up a dependency loop between ConstantMirror.h and
global_llvm_data.h, which was unnecessarily including ConstantMirror.h.

[Jeffrey Yasskin, 2009-12-07 19:10:34]

LGTM with a few tweaks. Thanks!

http://codereview.appspot.com/157130/diff/3001/4002
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/3001/4002#newcode3111
Lib/test/test_llvm.py:3111: sys.setbailerror(False)
On 2009/12/05 07:06:29, Reid Kleckner wrote:
> On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> > Is the code actually going to bail inside the get-hot loop? I was confused
> > reading this the first time thinking that get_foo() might have been compiled
> > before the loop, and the loop was going to re-compile it. If it's not going
to
> > bail until afterwards, probably move this to after the loop? Or assert that
> > __use_llvm__ is false before the loop?
> 
> I've moved all of the setbailerrors after the get-hot loops.  Does that
clarify
> it?

Clear enough. Thanks!

http://codereview.appspot.com/157130/diff/3001/4009
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/157130/diff/3001/4009#newcode601
Python/llvm_fbuilder.h:601: // llvm::Value versions of the above data.
On 2009/12/05 07:06:29, Reid Kleckner wrote:
> On 2009/12/05 01:38:42, Jeffrey Yasskin wrote:
> > These fields' names should be a simple transform of the PyObject field they
> > mirror, or they should have a comment saying what expression the value
> > represents. Are they computed with ConstantMirror, IntToPtr, or some other
> > method?
> 
> I'm not sure what you mean, they are all simple transforms of the other fields
> with a "v_" postfix.

Ah, so they are. I missed that guard_type_ existed.

> They're all created via EmbedPointer or ConstantInt::get.

http://codereview.appspot.com/157130/diff/3008/3010
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/3008/3010#newcode3318
Lib/test/test_llvm.py:3318: # cause a bail in the get_foo code.
... until we inline it. ;)

http://codereview.appspot.com/157130/diff/3008/3010#newcode3403
Lib/test/test_llvm.py:3403: # is called by checking
by checking what?

http://codereview.appspot.com/157130/diff/3008/3010#newcode3491
Lib/test/test_llvm.py:3491: tests = [OptimizationTests]
This looks stale.

http://codereview.appspot.com/157130/diff/3008/3013
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/3008/3013#newcode107
Objects/typeobject.c:107: /* Drop the weakref from the list.  */
This automatically happens when you decref and destroy the list, right?

[Reid Kleckner, 2009-12-07 20:02:29]

Great!  I'll commit as soon as I run one more regrtest.py and get some
instrumentation and benchmarking results for the commit message.

http://codereview.appspot.com/157130/diff/3008/3010
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/157130/diff/3008/3010#newcode3403
Lib/test/test_llvm.py:3403: # is called by checking
On 2009/12/07 19:10:34, Jeffrey Yasskin wrote:
> by checking what?

Done.

http://codereview.appspot.com/157130/diff/3008/3010#newcode3491
Lib/test/test_llvm.py:3491: tests = [OptimizationTests]
On 2009/12/07 19:10:34, Jeffrey Yasskin wrote:
> This looks stale.

Done.

http://codereview.appspot.com/157130/diff/3008/3013
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/3008/3013#newcode107
Objects/typeobject.c:107: /* Drop the weakref from the list.  */
On 2009/12/07 19:10:34, Jeffrey Yasskin wrote:
> This automatically happens when you decref and destroy the list, right?

Yes, but you suggested that I clear out the list before decrefing it in the last
round of review.  Is that not quite what you meant?

[Jeffrey Yasskin, 2009-12-07 20:36:23]

http://codereview.appspot.com/157130/diff/3008/3013
File Objects/typeobject.c (right):

http://codereview.appspot.com/157130/diff/3008/3013#newcode107
Objects/typeobject.c:107: /* Drop the weakref from the list.  */
On 2009/12/07 20:02:29, Reid Kleckner wrote:
> On 2009/12/07 19:10:34, Jeffrey Yasskin wrote:
> > This automatically happens when you decref and destroy the list, right?
> 
> Yes, but you suggested that I clear out the list before decrefing it in the
last
> round of review.  Is that not quite what you meant?

Oh! Not quite. :) I meant "clear out the field pointing at the list", not clear
out the list itself. So just having "type->tp_code_listeners = NULL;" up above,
like you do, was enough. Sorry for being confusing.