Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide iteration variable in list comprehensions #44586

Closed
ncoghlan opened this issue Feb 15, 2007 · 12 comments
Closed

Hide iteration variable in list comprehensions #44586

ncoghlan opened this issue Feb 15, 2007 · 12 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@ncoghlan
Copy link
Contributor

BPO 1660500
Nosy @gvanrossum, @birkenfeld, @ncoghlan
Files
  • list_comp_private_iter_vars.diff: Hide list comp iteration variables
  • new-set-comps.diff: + set comps
  • new-comps-updated.diff: Compatible with py3k SVN as of Apr 14
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/birkenfeld'
    closed_at = <Date 2008-01-06.22:29:46.197>
    created_at = <Date 2007-02-15.11:29:02.000>
    labels = ['interpreter-core']
    title = 'Hide iteration variable in list comprehensions'
    updated_at = <Date 2008-01-06.22:29:46.197>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2008-01-06.22:29:46.197>
    actor = 'admin'
    assignee = 'georg.brandl'
    closed = True
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2007-02-15.11:29:02.000>
    creator = 'ncoghlan'
    dependencies = []
    files = ['7763', '7764', '7765']
    hgrepos = []
    issue_num = 1660500
    keywords = ['patch']
    message_count = 12.0
    messages = ['51882', '51883', '51884', '51885', '51886', '51887', '51888', '51889', '51890', '51891', '51892', '51893']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'georg.brandl', 'ncoghlan']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1660500'
    versions = ['Python 3.0']

    @ncoghlan
    Copy link
    Contributor Author

    This patch hides the iteration variable in list comprehensions.
    It adds new tests (modelled on the generator expression tests) and also removes some del statements from the standard library (where code previously cleaned up its own namespace).
    The changes to symtable.[ch] are more significant than strictly necessary - I found it necessary to spend some time cleaning up the code in order to understand what was needed for the list comprehension changes. Given that the 2.x and 3.0 compilers have already diverged fairly significantly, I don't believe this will make the process of keeping them in sync any more difficult than it is already.

    Assigning to Georg for initial review (as his set comprehensions patch provided a great deal of inspiration for this one).

    @ncoghlan ncoghlan added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 15, 2007
    @ncoghlan ncoghlan added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 15, 2007
    @ncoghlan
    Copy link
    Contributor Author

    Speed measurements show a significant speed up over trunk & Python 2.4 for module/class level code:

    (Python 2.4)$ python -m timeit -s "seq=range(1000)" "[[x for x in seq] for y in seq]"
    10 loops, best of 3: 239 msec per loop
    (Python 2.x trunk)$ ./python -m timeit -s "seq=range(1000)" "[[x for x in seq] for y in seq]"
    10 loops, best of 3: 193 msec per loop
    (Python 3000)$ ./python -m timeit -s "seq=range(1000)" "[[x for x in seq] for y in seq]"
    10 loops, best of 3: 176 msec per loop

    This is almost certainly due to the variables and the list object becoming function locals.

    There is a slowdown inside a function (but we are still faster than Python 2.4):

    (Python 2.4)$ python -m timeit -s "seq=range(1000)" -s "def f(): return [[x for x in seq] for y in seq]" "f()"
    10 loops, best of 3: 259 msec per loop
    (Python 2.x trunk)$ ./python -m timeit -s "seq=range(1000)" -s "def f(): return [[x for x in seq] for y in seq]" "f()"
    10 loops, best of 3: 176 msec per loop
    (Python 3000)$ ./python -m timeit -s "seq=range(1000)" -s "def f(): return [[x for x in seq] for y in seq]" "f()"
    10 loops, best of 3: 185 msec per loop

    @ncoghlan
    Copy link
    Contributor Author

    For reference, the original set comprehensions patch & the py3k list discussion:
    http://www.python.org/sf/1548388
    http://mail.python.org/pipermail/python-3000/2006-December/005188.html

    Note that the current patch ended up looking a *lot* like the original one (the main difference specific to list comprehensions is that the temporary list is built in the inner scope and then returned rather than being passed in as an argument. Additionally, the code has been unified only for the symtable stage - the AST and compilation stages still use separate code for listcomps and genexps).

    It turns out that there are some really curly scoping problems that using a nested function deals with automatically (see the new test_listcomps.py in the patch for examples). Having a more efficient mechanism specifically for 'transient' scopes like this is an interesting idea, but it's far from easy (mainly because the variables in the scope still need to be properly visible in scopes it *contains*).

    Adding set comprehensions on top of this patch should be pretty straightforward.

    @birkenfeld
    Copy link
    Member

    Okay, I looked at the patch, and apart from a refleak in one of the compiler methods I couldn't find anything wrong.
    I then manually applied the rest of my set comprehension patch. The result is attached.

    Grammar, AST and compilation for comprehensions are now really unified.

    It passes the tests for listcomps, genexps and setcomps.

    I couldn't check properly for refleaks since e.g. test_genexps leaks in the branch head, as well as some other tests. I'm currently searching for the offending revision(s)...
    File Added: new-set-comps.diff

    @gvanrossum
    Copy link
    Member

    Can't say that slowdown bothers me much, if it's typical.

    However I think you need to do a svn up in your workspace and regenerate the patch; I get FAILED message from patch on all the generated files and a few others: graminit.[ch], symtable.[ch], Python-ast.c, symbol.py.

    @ncoghlan
    Copy link
    Contributor Author

    The patch for 'nonlocal' was applied since Georg & I started working on this. It's going to take me a bit of fiddling to update the patch so that the parser/compiler stage play well with each other (I really should have just believed the patch utility when it reported a conflict trying to patch symtable.c).

    @ncoghlan
    Copy link
    Contributor Author

    I've uploaded a new version of the patch that is compatible with the py3k SVN branch as of April 14.

    The patch still includes the various cleanups I made to the symbol table processing while working out how to make this change (use sets where appropriate, avoid using the same variable name to refer to completely different things, make a couple of syntax error messages more explicit).

    The slowdown should be fixed at the cost of a single function call - the shorter the sequence being iterated over, the greater the percentage slowdown that will be. The speed of generator expressions should actually be (very) marginally increased as they now skip the SETUP_LOOP/POP_BLOCK steps in the same fashion as list comprehensions always have.
    File Added: new-comps-updated.diff

    @gvanrossum
    Copy link
    Member

    Assuming all tests pass in a debug build and you don't see any leaks with "regrtest.py -R 4:3:", just check it in! I've been waiting long enough for this. Speedups are for post 3.0a1.

    @birkenfeld
    Copy link
    Member

    I applied this and ran regrtest -R. Found no refleaks, and the only tests that failed were compiler and transformer, as expected.

    @gvanrossum
    Copy link
    Member

    OK, I think Nick can check it in himself, right Nick?

    @ncoghlan
    Copy link
    Contributor Author

    test_compiler and test_transformer are failing due to the fact that the compiler package still needs some TLC to catch up with the Grammar changes.

    test_logging, test_tcl and test_structmembers all fail when run with -R 4:3:. However, these failures don't appear to be due to this change. More importantly, no leaks are reported in the new tests that are part of this update (test_listcomps, test_setcomps).

    Committed as rev 54835.

    @birkenfeld
    Copy link
    Member

    The compiler package has been thrown out of Py3k, so there's no reason to leave this open anymore.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants