Ticket #123 (closed defect: fixed)

Opened 2 months ago

Last modified 1 month ago

Regression due to PyDict_Next optimization

Reported by: jasone Assigned to:
Priority: major Milestone: 0.11
Component: Code Generation Version:
Keywords: Cc:

Description

These changesets introduce a code generation bug that causes repeated dictionary iteration to fail (using Python 2.6 on 32- and 64-bit Ubuntu 8.04): 

changeset:   1344:7a102bc50f8f
user:        Stefan Behnel <scoder@users.berlios.de>
date:        Sun Nov 16 22:45:33 2008 +0100
summary:     integrate new iter-dict transform

changeset:   1343:db3eb81258f4
user:        Stefan Behnel <scoder@users.berlios.de>
date:        Sun Nov 16 22:45:12 2008 +0100
summary:     new transform that converts for-in-dict.iter*() into a while-loop over PyDict_Next(), which makes the loop 30-50% faster

The attached test case demonstrates the problem.

Attachments

foo.pyx (132 bytes) - added by jasone on 11/19/2008 01:38:59 PM.
PyDict_Next (0.7 kB) - added by jasone on 11/20/2008 10:30:58 AM.
Patch to use local temp variable

Change History

11/19/2008 01:38:59 PM changed by jasone

  • attachment foo.pyx added.

11/20/2008 10:30:58 AM changed by jasone

  • attachment PyDict_Next added.

Patch to use local temp variable

11/20/2008 10:33:18 AM changed by jasone

The attached patch fixes iteration by using a local temporary variable, rather than using the iterator counter. I think the real problem is that some tree tranform is removing the node that initializes the iterator counter (perhaps it's viewed as a redundant global variable initialization), but finding that problem is beyond my understanding of Cython at this point.

11/25/2008 11:31:23 AM changed by scoder

  • owner changed from somebody to scoder.

11/25/2008 12:43:13 PM changed by scoder

  • owner deleted.

Yes, it looks like a problem with the initialisation. The same temp node gets reused here, but it's only initialised once.

I applied your work-around by now, but the real problem is elsewhere in the temp code.

11/29/2008 03:10:09 PM changed by dagss

I have now changed IteratorNode so that the counter is considered an implementation detail/"temp register", rather than a subnode in the tree. Thus it shouldn't be used by transforms, and the consequence is that this patch now does the entirely correct thing.

I recommend closing as "fixed", but waiting for review.

11/29/2008 03:11:27 PM changed by dagss

See #124 for relevant patches.

11/29/2008 09:25:07 PM changed by scoder

  • status changed from new to closed.
  • resolution set to fixed.

Yes, that's the right thing to do.