Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions Lib/test/test_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -2063,7 +2063,7 @@ def test_multiline_async_generator_expression(self):

def test_multiline_list_comprehension(self):
snippet = textwrap.dedent("""\
[(x,
_ = [(x,
2*x)
for x
in [1,2,3] if (x > 0
Expand All @@ -2073,14 +2073,14 @@ def test_multiline_list_comprehension(self):
compiled_code, _ = self.check_positions_against_ast(snippet)
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'LIST_APPEND',
line=1, end_line=2, column=1, end_column=8, occurrence=1)
line=1, end_line=2, column=5, end_column=8, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=1, end_line=2, column=1, end_column=8, occurrence=1)
line=1, end_line=2, column=5, end_column=8, occurrence=1)

def test_multiline_async_list_comprehension(self):
snippet = textwrap.dedent("""\
async def f():
[(x,
_ = [(x,
2*x)
async for x
in [1,2,3] if (x > 0
Expand All @@ -2093,15 +2093,15 @@ async def f():
compiled_code = g['f'].__code__
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'LIST_APPEND',
line=2, end_line=3, column=5, end_column=12, occurrence=1)
line=2, end_line=3, column=9, end_column=12, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=2, end_line=3, column=5, end_column=12, occurrence=1)
line=2, end_line=3, column=9, end_column=12, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE',
line=2, end_line=7, column=4, end_column=36, occurrence=1)
line=2, end_line=2, column=4, end_column=5, occurrence=1)

def test_multiline_set_comprehension(self):
snippet = textwrap.dedent("""\
{(x,
_ = {(x,
2*x)
for x
in [1,2,3] if (x > 0
Expand All @@ -2111,9 +2111,9 @@ def test_multiline_set_comprehension(self):
compiled_code, _ = self.check_positions_against_ast(snippet)
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'SET_ADD',
line=1, end_line=2, column=1, end_column=8, occurrence=1)
line=1, end_line=2, column=5, end_column=8, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=1, end_line=2, column=1, end_column=8, occurrence=1)
line=1, end_line=2, column=5, end_column=8, occurrence=1)

def test_multiline_async_set_comprehension(self):
snippet = textwrap.dedent("""\
Expand All @@ -2139,7 +2139,7 @@ async def f():

def test_multiline_dict_comprehension(self):
snippet = textwrap.dedent("""\
{x:
_ = {x:
2*x
for x
in [1,2,3] if (x > 0
Expand All @@ -2149,14 +2149,14 @@ def test_multiline_dict_comprehension(self):
compiled_code, _ = self.check_positions_against_ast(snippet)
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'MAP_ADD',
line=1, end_line=2, column=1, end_column=7, occurrence=1)
line=1, end_line=2, column=5, end_column=7, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=1, end_line=2, column=1, end_column=7, occurrence=1)
line=1, end_line=2, column=5, end_column=7, occurrence=1)

def test_multiline_async_dict_comprehension(self):
snippet = textwrap.dedent("""\
async def f():
{x:
_ = {x:
2*x
async for x
in [1,2,3] if (x > 0
Expand All @@ -2169,11 +2169,11 @@ async def f():
compiled_code = g['f'].__code__
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'MAP_ADD',
line=2, end_line=3, column=5, end_column=11, occurrence=1)
line=2, end_line=3, column=9, end_column=11, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=2, end_line=3, column=5, end_column=11, occurrence=1)
line=2, end_line=3, column=9, end_column=11, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE',
line=2, end_line=7, column=4, end_column=36, occurrence=1)
line=2, end_line=2, column=4, end_column=5, occurrence=1)

def test_matchcase_sequence(self):
snippet = textwrap.dedent("""\
Expand Down
36 changes: 36 additions & 0 deletions Lib/test/test_compiler_codegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,3 +192,39 @@ def test_frozenset_optimization(self):
('RETURN_VALUE', None)
]
self.codegen_test(snippet, expected)

def test_no_target_comp_optimization(self):
Comment thread
sergey-miryanov marked this conversation as resolved.
Outdated
Comment thread
ZeroIntensity marked this conversation as resolved.
Outdated
snippet = "[i for i in range(10)]"
expected = [
('RESUME', 0),
('ANNOTATIONS_PLACEHOLDER', None),
('LOAD_NAME', 0),
('PUSH_NULL', None),
('LOAD_CONST', 0),
('CALL', 1),
('LOAD_FAST_AND_CLEAR', 0),
('SWAP', 2),
('SETUP_FINALLY', 20),
('COPY', 1),
('GET_ITER', 0),
('FOR_ITER', 16),
('STORE_FAST', 0),
('LOAD_FAST', 0),
('POP_TOP', None),
('JUMP', 11),
('END_FOR', None),
('POP_ITER', None),
('POP_BLOCK', None),
('JUMP_NO_INTERRUPT', 25),
('SWAP', 2),
('POP_TOP', None),
('SWAP', 2),
('STORE_FAST_MAYBE_NULL', 0),
('RERAISE', 0),
('SWAP', 2),
('STORE_FAST_MAYBE_NULL', 0),
('POP_TOP', None),
('LOAD_CONST', 1),
('RETURN_VALUE', None),
]
self.codegen_test(snippet, expected)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Avoid creating :class:`list`, :class:`set`, and :class:`dict` objects in
comprehensions when the comprehension is not used as a value.
106 changes: 87 additions & 19 deletions Python/codegen.c
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,18 @@ typedef enum {
ITERATOR_ON_STACK = 2,
} IterStackPosition;

static int
codegen_listcomp_impl(compiler *c, expr_ty e, bool avoid_creation);
static int
codegen_setcomp_impl(compiler *c, expr_ty e, bool avoid_creation);
static int
codegen_dictcomp_impl(compiler *c, expr_ty e, bool avoid_creation);
static int codegen_sync_comprehension_generator(
compiler *c, location loc,
asdl_comprehension_seq *generators, int gen_index,
int depth,
expr_ty elt, expr_ty val, int type,
IterStackPosition iter_pos);
IterStackPosition iter_pos, bool avoid_creation);

static int codegen_async_comprehension_generator(
compiler *c, location loc,
Expand Down Expand Up @@ -3084,6 +3090,20 @@ codegen_stmt_expr(compiler *c, location loc, expr_ty value)
return SUCCESS;
}

if (value->kind == ListComp_kind) {
/* Optimization: Don't bother creating structures if they won't be
* used. */
return codegen_listcomp_impl(c, value, /*avoid_creation=*/true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nicer if we were not calling codegen_listcomp_impl here but rather setting a boolean on the compiler_unit struct to track whether we are in the topmost expression or not. What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not opposed to the idea, but it feels slightly weird in cases where the thing being compiled isn't an expression. A compiler->has_no_target flag in a FunctionDef, for example, has no real meaning (because compiler->has_no_target == 0 would imply "has target", but that doesn't make sense for a statement). I do think it'll make future optimizations like this easier, though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a flag like is_subexpression.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I implement that? I can't do like METADATA(c)->u_is_subexpression = 1 in codegen_stmt_expr because that would also affect listcomps inside the one we're optimizing (like [x for x in [y for y in z]]). I could add a stack or something like that, but that would significantly increase the complexity of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this:

static int
codegen_visit_expr(compiler *c, expr_ty e)
{
    if (Py_EnterRecursiveCall(" during compilation")) {
        return ERROR;
    }
    return codegen_visit_expr_impl(c, e, false);
}

static int
codegen_visit_unused_expr(compiler *c, expr_ty e)
{
    return codegen_visit_expr_impl(c, e, true);
}

static int
codegen_visit_expr_impl(compiler *c, expr_ty e, bool result_is_unused)
{
#define VISIT_UNUSED(C, TYPE, V) \
    RETURN_IF_ERROR(codegen_visit_unused_ ## TYPE((C), (V)))

and then in codegen_stmt_expr use VISIT_UNUSED instead of VISIT?

Then codegen_stmt_expr doesn't need to do anything special for ListComp_kind.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea. I just pushed it, let me know what you think.

}

if (value->kind == SetComp_kind) {
return codegen_setcomp_impl(c, value, /*avoid_creation=*/true);
}

if (value->kind == DictComp_kind) {
return codegen_dictcomp_impl(c, value, /*avoid_creation=*/true);
}

VISIT(c, expr, value);
ADDOP(c, NO_LOCATION, POP_TOP); /* artificial */
return SUCCESS;
Expand Down Expand Up @@ -4541,7 +4561,7 @@ codegen_comprehension_generator(compiler *c, location loc,
asdl_comprehension_seq *generators, int gen_index,
int depth,
expr_ty elt, expr_ty val, int type,
IterStackPosition iter_pos)
IterStackPosition iter_pos, bool avoid_creation)
{
comprehension_ty gen;
gen = (comprehension_ty)asdl_seq_GET(generators, gen_index);
Expand All @@ -4552,7 +4572,7 @@ codegen_comprehension_generator(compiler *c, location loc,
} else {
return codegen_sync_comprehension_generator(
c, loc, generators, gen_index, depth, elt, val, type,
iter_pos);
iter_pos, avoid_creation);
}
}

Expand All @@ -4561,7 +4581,7 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
asdl_comprehension_seq *generators,
int gen_index, int depth,
expr_ty elt, expr_ty val, int type,
IterStackPosition iter_pos)
IterStackPosition iter_pos, bool avoid_creation)
{
/* generate code for the iterator, then each of the ifs,
and then write to the element */
Expand Down Expand Up @@ -4629,7 +4649,7 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
RETURN_IF_ERROR(
codegen_comprehension_generator(c, loc,
generators, gen_index, depth,
elt, val, type, ITERABLE_IN_LOCAL));
elt, val, type, ITERABLE_IN_LOCAL, avoid_creation));
}

location elt_loc = LOC(elt);
Expand All @@ -4639,6 +4659,7 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
/* comprehension specific code */
switch (type) {
case COMP_GENEXP:
assert(!avoid_creation);
if (elt->kind == Starred_kind) {
NEW_JUMP_TARGET_LABEL(c, unpack_start);
NEW_JUMP_TARGET_LABEL(c, unpack_end);
Expand All @@ -4660,6 +4681,11 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
}
break;
case COMP_LISTCOMP:
if (avoid_creation) {
VISIT(c, expr, elt);
ADDOP(c, elt_loc, POP_TOP);
break;
}
Comment thread
iritkatriel marked this conversation as resolved.
if (elt->kind == Starred_kind) {
VISIT(c, expr, elt->v.Starred.value);
ADDOP_I(c, elt_loc, LIST_EXTEND, depth + 1);
Expand All @@ -4670,6 +4696,11 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
}
break;
case COMP_SETCOMP:
if (avoid_creation) {
VISIT(c, expr, elt);
ADDOP(c, elt_loc, POP_TOP);
break;
}
if (elt->kind == Starred_kind) {
VISIT(c, expr, elt->v.Starred.value);
ADDOP_I(c, elt_loc, SET_UPDATE, depth + 1);
Expand All @@ -4680,6 +4711,11 @@ codegen_sync_comprehension_generator(compiler *c, location loc,
}
break;
case COMP_DICTCOMP:
if (avoid_creation) {
VISIT(c, expr, elt);
ADDOP(c, elt_loc, POP_TOP);
break;
}
if (val == NULL) {
/* unpacking (**) case */
VISIT(c, expr, elt);
Expand Down Expand Up @@ -4773,7 +4809,7 @@ codegen_async_comprehension_generator(compiler *c, location loc,
RETURN_IF_ERROR(
codegen_comprehension_generator(c, loc,
generators, gen_index, depth,
elt, val, type, 0));
elt, val, type, 0, false));
}

location elt_loc = LOC(elt);
Expand Down Expand Up @@ -4996,7 +5032,7 @@ pop_inlined_comprehension_state(compiler *c, location loc,
static int
codegen_comprehension(compiler *c, expr_ty e, int type,
identifier name, asdl_comprehension_seq *generators, expr_ty elt,
expr_ty val)
expr_ty val, bool avoid_creation)
{
PyCodeObject *co = NULL;
_PyCompile_InlinedComprehensionState inline_state = {NULL, NULL, NULL, NO_LABEL};
Expand Down Expand Up @@ -5070,20 +5106,27 @@ codegen_comprehension(compiler *c, expr_ty e, int type,
goto error_in_scope;
}

ADDOP_I(c, loc, op, 0);
if (is_inlined) {
ADDOP_I(c, loc, SWAP, 2);
if (!avoid_creation) {
ADDOP_I(c, loc, op, 0);
if (is_inlined) {
ADDOP_I(c, loc, SWAP, 2);
}
} else {
ADDOP_I(c, loc, COPY, 1);
Comment thread
iritkatriel marked this conversation as resolved.
}
}
if (codegen_comprehension_generator(c, loc, generators, 0, 0,
elt, val, type, iter_state) < 0) {
elt, val, type, iter_state, avoid_creation) < 0) {
goto error_in_scope;
}

if (is_inlined) {
if (pop_inlined_comprehension_state(c, loc, &inline_state)) {
goto error;
}
if (avoid_creation) {
ADDOP(c, loc, POP_TOP);
}
return SUCCESS;
}

Expand Down Expand Up @@ -5118,6 +5161,8 @@ codegen_comprehension(compiler *c, expr_ty e, int type,
ADD_YIELD_FROM(c, loc, 1);
}

assert(!avoid_creation);

return SUCCESS;
error_in_scope:
if (!is_inlined) {
Expand All @@ -5133,44 +5178,67 @@ codegen_comprehension(compiler *c, expr_ty e, int type,
}

static int
codegen_genexp(compiler *c, expr_ty e)
codegen_genexp_impl(compiler *c, expr_ty e, bool avoid_creation)
{
assert(e->kind == GeneratorExp_kind);
_Py_DECLARE_STR(anon_genexpr, "<genexpr>");
return codegen_comprehension(c, e, COMP_GENEXP, &_Py_STR(anon_genexpr),
e->v.GeneratorExp.generators,
e->v.GeneratorExp.elt, NULL);
e->v.GeneratorExp.elt, NULL, avoid_creation);
}

static int
codegen_listcomp(compiler *c, expr_ty e)
codegen_genexp(compiler *c, expr_ty e)
{
return codegen_genexp_impl(c, e, false);
}

static int
codegen_listcomp_impl(compiler *c, expr_ty e, bool avoid_creation)
{
assert(e->kind == ListComp_kind);
_Py_DECLARE_STR(anon_listcomp, "<listcomp>");
return codegen_comprehension(c, e, COMP_LISTCOMP, &_Py_STR(anon_listcomp),
e->v.ListComp.generators,
e->v.ListComp.elt, NULL);
e->v.ListComp.elt, NULL, avoid_creation);
}

static int
codegen_setcomp(compiler *c, expr_ty e)
codegen_listcomp(compiler *c, expr_ty e)
{
return codegen_listcomp_impl(c, e, false);
}

static int
codegen_setcomp_impl(compiler *c, expr_ty e, bool avoid_creation)
{
assert(e->kind == SetComp_kind);
_Py_DECLARE_STR(anon_setcomp, "<setcomp>");
return codegen_comprehension(c, e, COMP_SETCOMP, &_Py_STR(anon_setcomp),
e->v.SetComp.generators,
e->v.SetComp.elt, NULL);
e->v.SetComp.elt, NULL, avoid_creation);
}

static int
codegen_setcomp(compiler *c, expr_ty e)
{
return codegen_setcomp_impl(c, e, false);
}

static int
codegen_dictcomp(compiler *c, expr_ty e)
codegen_dictcomp_impl(compiler *c, expr_ty e, bool avoid_creation)
{
assert(e->kind == DictComp_kind);
_Py_DECLARE_STR(anon_dictcomp, "<dictcomp>");
return codegen_comprehension(c, e, COMP_DICTCOMP, &_Py_STR(anon_dictcomp),
e->v.DictComp.generators,
e->v.DictComp.key, e->v.DictComp.value);
e->v.DictComp.key, e->v.DictComp.value, avoid_creation);
}

static int
codegen_dictcomp(compiler *c, expr_ty e)
{
return codegen_dictcomp_impl(c, e, false);
}


Expand Down
Loading