Ticket #123 (closed defect: fixed)

Opened 6 years ago

Last modified 5 years ago

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

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

Change History

Changed 6 years ago by jasone

Changed 6 years ago by jasone

Patch to use local temp variable

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.

Changed 6 years ago by scoder

  • owner changed from somebody to scoder

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.

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.

Changed 6 years ago by dagss

See #124 for relevant patches.

Changed 6 years ago by scoder

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

Yes, that's the right thing to do.

Changed 5 years ago by robertwb

  • status changed from closed to reopened
  • resolution fixed deleted

Changed 5 years ago by robertwb

  • owner set to jasone
  • status changed from reopened to new

Changed 5 years ago by robertwb

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