Opened 6 years ago

Closed 6 years ago

#123 closed defect (fixed)

Regression due to PyDict_Next optimization

Reported by: jasone Owned by: jasone
Priority: major Milestone: 0.11
Component: Code Generation 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 (2)

foo.pyx (132 bytes) - added by jasone 6 years ago.
PyDict_Next (707 bytes) - added by jasone 6 years ago.
Patch to use local temp variable

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by jasone

Changed 6 years ago by jasone

Patch to use local temp variable

comment:1 Changed 6 years ago 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.

comment:2 Changed 6 years ago by scoder

  • Owner changed from somebody to scoder

comment:3 Changed 6 years ago by scoder

  • Owner scoder 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.

comment:4 Changed 6 years ago 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.

comment:5 Changed 6 years ago by dagss

See #124 for relevant patches.

comment:6 Changed 6 years ago by scoder

  • Resolution set to fixed
  • Status changed from new to closed

Yes, that's the right thing to do.

comment:7 Changed 6 years ago by robertwb

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 Changed 6 years ago by robertwb

  • Owner set to jasone
  • Status changed from reopened to new

comment:9 Changed 6 years ago by robertwb

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.