gh-123080: Additional ast optimizations #123081

wrongnull · 2024-08-16T19:09:37Z

This allows some expression templates to produce less bytecode and perform more folding operations in the fold_compare function.
For instance

>>> from dis import dis
>>> dis("1 in [1, 2, 3, 4]")
  0           RESUME                   0

  1           RETURN_CONST             0 (True)
>>> dis("1 == 1")
  0           RESUME                   0

  1           RETURN_CONST             0 (True)
>>>

Issue: Missing some obvious ast optimizations #123080

wrongnull · 2024-08-16T19:13:54Z

I guess there is no need to update news for this

terryjreedy · 2024-08-16T20:38:19Z

I believe that changing bytecode requires News and What's New items.

wrongnull · 2024-08-17T08:50:42Z

I'll convert it to draft so far because of tests segfaults. Also I'll wait until #122667 will be merged

wrongnull · 2024-08-18T15:16:31Z

For now, everything works fine and all the previous optimizations are preserved. I'll wait for tests of ast optimizations to be merged and then I'll add test coverage for it.

pablogsal · 2024-08-19T19:07:31Z

Python/ast_opt.c

+
+        PyObject *newval = PyTuple_New(elts_len);
+        if (newval == NULL) {
+            if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) {


There is no need to match against PyExc_KeyboardInterrupt here. Just treat this like a regular error and bubble up

pablogsal · 2024-08-19T19:08:12Z

Python/ast_opt.c

+        for (Py_ssize_t j = 0; j < elts_len; j++) {
+            expr_ty e = (expr_ty)asdl_seq_GET(elts, j);
+            if (e->kind != Constant_kind) {
+                PyObject_GC_UnTrack(newval);


Why are you calling directly these GC functions? Just call a normal Py_DECREF when you are done with it

pablogsal · 2024-08-19T19:08:21Z

Python/ast_opt.c

+            if (is_lhs_constant &&
+                (v == (node->v.Compare.left->v.Constant.value)))
+            {
+                PyObject_GC_UnTrack(newval);


Same as https://github.com/python/cpython/pull/123081/files#r1722232517

pablogsal · 2024-08-19T19:08:36Z

Python/ast_opt.c

        }
+
+        if (arg->kind == Set_kind)


PEP 7: Add braces

pablogsal · 2024-08-19T19:08:56Z

Python/ast_opt.c

        }
+
+        if (arg->kind == Set_kind)
+            Py_SETREF(newval, PyFrozenSet_New(newval));


PyFrozenSet_New can fail, you need to check for errors and handle the failure

This line was copied from fold_iter. So, fold_iter has the same issue (no error handling for NULL values on calls to PyFrozenSet_New). Do you think it's OK to address this issue in this PR?

pablogsal · 2024-08-19T19:09:11Z

Python/ast_opt.c

-            return 0;
+        if (arg->kind == List_kind) {
+            asdl_expr_seq *list_elts = arg->v.List.elts;
+            if (has_starred(list_elts))


PEP 7: add braces

pablogsal · 2024-08-19T19:12:47Z

Python/ast_opt.c

+             (arg->kind == Constant_kind) &&
+             is_lhs_constant)
+    {
+        if ((node->v.Compare.left->v.Constant.value) == (arg->v.Constant.value))


PEP 7: Add braces

pablogsal · 2024-08-19T19:13:02Z

Python/ast_opt.c


    ops = node->v.Compare.ops;
    args = node->v.Compare.comparators;
    /* Change literal list or set in 'in' or 'not in' into
       tuple or frozenset respectively. */
    i = asdl_seq_LEN(ops) - 1;
    int op = asdl_seq_GET(ops, i);
+    arg = asdl_seq_GET(args, i);
+    _Bool is_lhs_constant = (node->v.Compare.left->kind) == Constant_kind;
+    //lhs_const = node->v.Compare.left->v.Constant.value;


Why is this commented out?

Sorry I leave it by accident

pablogsal

Left some comments

bedevere-app · 2024-08-19T19:20:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Eclips4 · 2024-08-20T04:57:33Z

Python/ast_opt.c

+                PyObject_GC_UnTrack(newval);
+                PyObject_GC_Del(newval);


Suggested change

PyObject_GC_UnTrack(newval);

PyObject_GC_Del(newval);

Py_DECREF(newval);

Eclips4 · 2024-08-20T04:59:28Z

Python/ast_opt.c

        }
+
+        if (arg->kind == Set_kind)
+            Py_SETREF(newval, PyFrozenSet_New(newval));


This line was copied from fold_iter. So, fold_iter has the same issue (no error handling for NULL values on calls to PyFrozenSet_New). Do you think it's OK to address this issue in this PR?

pablogsal · 2024-08-20T11:03:19Z

Do you think it's OK to address this issue in this PR?

I prefer to do that separately because we want to back port it. I will take care of that

Implement desired functionality

182dc6d

wrongnull requested a review from isidentical as a code owner August 16, 2024 19:09

bedevere-app bot mentioned this pull request Aug 16, 2024

Missing some obvious ast optimizations #123080

Closed

bedevere-app bot added the awaiting review label Aug 16, 2024

terryjreedy requested a review from pablogsal August 16, 2024 20:38

Eclips4 self-assigned this Aug 17, 2024

wrongnull marked this pull request as draft August 17, 2024 08:51

bedevere-app bot removed the awaiting review label Aug 17, 2024

wrongnull added 2 commits August 18, 2024 16:22

Necessary fixes

d4a97b0

Add one more check

893e41d

wrongnull marked this pull request as ready for review August 18, 2024 15:14

bedevere-app bot added the awaiting review label Aug 18, 2024

convert dict literal to set

fa2bf51

pablogsal reviewed Aug 19, 2024

View reviewed changes

Python/ast_opt.c Outdated

}

if (arg->kind == Set_kind)

Copy link

Member

pablogsal Aug 19, 2024

Choose a reason for hiding this comment

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

PEP 7: Add braces

pablogsal reviewed Aug 19, 2024

View reviewed changes

pablogsal requested changes Aug 19, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Aug 19, 2024

Eclips4 reviewed Aug 20, 2024

View reviewed changes

wrongnull added 4 commits August 20, 2024 08:55

Add more braces and eliminate commented out code

7ee9a65

Use Py_DECREF instead of GC functions

af11b23

KeyboardInterrupt is no loanger handled

4add36d

Handle possible PyFrozenSet_New failure

04192e2

wrongnull requested review from pablogsal and Eclips4 August 20, 2024 06:20

This was referenced Aug 20, 2024

Missing error check in ast_opt.c #123171

Closed

gh-123171: Fix error paths in ast_opt.c for for_iter optimisations #123172

Closed

serhiy-storchaka closed this Aug 21, 2024

	PyObject_GC_UnTrack(newval);
	PyObject_GC_Del(newval);
	Py_DECREF(newval);

Uh oh!

gh-123080: Additional ast optimizations #123081

gh-123080: Additional ast optimizations #123081

Uh oh!

Conversation

wrongnull commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wrongnull commented Aug 16, 2024

Uh oh!

terryjreedy commented Aug 16, 2024

Uh oh!

wrongnull commented Aug 17, 2024

Uh oh!

wrongnull commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Aug 20, 2024

Uh oh!

Uh oh!

wrongnull commented Aug 16, 2024 •

edited

Loading

wrongnull commented Aug 18, 2024 •

edited

Loading