Lurking CPU eater in Transience

Bug #143268 reported by Evan Simpson
2
Affects Status Importance Assigned to Milestone
Zope 2
Invalid
Medium
Chris McDonough

Bug Description

I foolishly set the timeout of a Transient Object Container to a very high value while leaving the timeout resolution at 20. Subsequent profiling revealed that Transience.py's getTimeslices was using 100% CPU for more than a second every time it was called, which was frequently. This was due to it inserting tens of thousands of elements into the head of an empty list, one at a time. While I learned my lesson, and bumped the resolution up to match the timeout, I also decided to fix getTimeslices. A patch is attached.

Revision history for this message
Evan Simpson (evan-4-am) wrote :
  • p Edit (634 bytes, text/plain)
Revision history for this message
Chris McDonough (chrism-plope) wrote :

> This was due to it inserting tens of thousands of elements into the head
> of an empty list, one at a time. While I learned my lesson, and bumped
> the resolution up to match the timeout, I also decided to fix
> getTimeslices. A patch is attached.

Cool, thanks! I'm wondering, though, I've always heard it was bad to mutate a list while iterating over it. Does that only matter if you change the size of the list while doing so?

Revision history for this message
Evan Simpson (evan-4-am) wrote :

Inserting or deleting elements or replacing a slice that spans the current element in a for-loop is generally a bad idea. Changing the value of a list element is fine.

Revision history for this message
Tim Peters (tim-one) wrote :

Everything about mutating Python lists while iterating over them is defined. It can be *surprising* if the list ever changes size while that's going on, but it's still well-defined even then. There's nothing surprising about what happens if the list doesn't change size.

That said, variables named "l" should be outlawed. In Courier New, there's only one pixel to distinguish the letter from the digit, and I couldn't understand at first why the patched code returned the digit 1.

Suggest something like

result = [x * period + begin for x in xrange(n)]
result.reverse()
return result

instead. Then it's readable, fast, and nobody can get confused about mutation-while-iterating.

OTOH, if ``begin`` and ``period`` are integers, and period>0, it's really just doing this one-liner:

return range(begin + period*(n-1), begin-1, -period)

Revision history for this message
Evan Simpson (evan-4-am) wrote :

Absolutely. My "bad idea" comment refers to the likelihood of writing code with surprising (and therefore incorrect) results.

Your one-liner correctly captures the intent of this function, and should be used.

Revision history for this message
Tim Peters (tim-one) wrote :

Evan, we were writing followups at the same time -- I didn't see your "bad idea" followup when I was writing mine, I was just replying to Chris. That said, I agree with your "bad idea" <wink>.

Revision history for this message
Colin Watson (cjwatson) wrote :

The zope2 project on Launchpad has been archived at the request of the Zope developers (see https://answers.launchpad.net/launchpad/+question/683589 and https://answers.launchpad.net/launchpad/+question/685285). If this bug is still relevant, please refile it at https://github.com/zopefoundation/zope2.

Changed in zope2:
status: Confirmed → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.