Skip to content
New issue

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

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

Already on GitHub? # to your account

Reduce exception table size by 1 entry for every with-block #409

Closed
iritkatriel opened this issue Jun 8, 2022 · 7 comments
Closed

Reduce exception table size by 1 entry for every with-block #409

iritkatriel opened this issue Jun 8, 2022 · 7 comments
Assignees

Comments

@iritkatriel
Copy link
Collaborator

The dis for this function:

def f1(cm):
    with cm:
        x = 1
    x = 2

is this:

 11           0 RESUME                   0

 12           2 LOAD_FAST                0 (cm)
              4 BEFORE_WITH
              6 POP_TOP

 13           8 LOAD_CONST               1 (1)
             10 STORE_FAST               1 (x)

 12          12 LOAD_CONST               0 (None)
             14 LOAD_CONST               0 (None)
             16 LOAD_CONST               0 (None)
             18 CALL                     2
             28 POP_TOP

 14          30 LOAD_CONST               2 (2)
             32 STORE_FAST               1 (x)
             34 LOAD_CONST               0 (None)
             36 RETURN_VALUE

 12     >>   38 PUSH_EXC_INFO
             40 WITH_EXCEPT_START
             42 POP_JUMP_FORWARD_IF_TRUE     4 (to 52)
             44 RERAISE                  2
        >>   46 COPY                     3
             48 POP_EXCEPT
             50 RERAISE                  1
        >>   52 POP_TOP
             54 POP_EXCEPT
             56 POP_TOP
             58 POP_TOP

 14          60 LOAD_CONST               2 (2)
             62 STORE_FAST               1 (x)
             64 LOAD_CONST               0 (None)
             66 RETURN_VALUE
ExceptionTable:
  6 to 10 -> 38 [1] lasti
  38 to 44 -> 46 [3] lasti
  52 to 52 -> 46 [3] lasti

Note the last row of the exception table: inst 52 goes to 46, same as 38-44. We can reorder the code so that this block is consecutive, and we save one entry in the exception table for each with/async with:

 11           0 RESUME                   0

 12           2 LOAD_FAST                0 (cm)
              4 BEFORE_WITH
              6 POP_TOP

 13           8 LOAD_CONST               1 (1)
             10 STORE_FAST               1 (x)

 12          12 LOAD_CONST               0 (None)
             14 LOAD_CONST               0 (None)
             16 LOAD_CONST               0 (None)
             18 CALL                     2
             28 POP_TOP

 14          30 LOAD_CONST               2 (2)
             32 STORE_FAST               1 (x)
             34 LOAD_CONST               0 (None)
             36 RETURN_VALUE

 12     >>   38 PUSH_EXC_INFO
             40 WITH_EXCEPT_START
             42 POP_JUMP_FORWARD_IF_TRUE     1 (to 46)
             44 RERAISE                  2
        >>   46 POP_TOP
             48 POP_EXCEPT
             50 POP_TOP
             52 POP_TOP

 14          54 LOAD_CONST               2 (2)
             56 STORE_FAST               1 (x)
             58 LOAD_CONST               0 (None)
             60 RETURN_VALUE
        >>   62 COPY                     3
             64 POP_EXCEPT
             66 RERAISE                  1
ExceptionTable:
  6 to 10 -> 38 [1] lasti
  38 to 46 -> 62 [3] lasti
@iritkatriel
Copy link
Collaborator Author

PR at python/cpython#93622.

@iritkatriel iritkatriel moved this to In Progress in Fancy CPython Board Jun 8, 2022
@iritkatriel iritkatriel self-assigned this Jun 8, 2022
@iritkatriel iritkatriel moved this from In Progress to In Review in Fancy CPython Board Jun 8, 2022
@brandtbucher
Copy link
Member

Maybe I'm missing something, but why is POP_TOP in the table at all? It can't raise, can it?

@iritkatriel
Copy link
Collaborator Author

It does a decref so I think it can.

@brandtbucher
Copy link
Member

Exceptions in finalizers aren’t propagated. I think a lot of code would be incorrect if Py_DECREF could raise, haha.

@iritkatriel
Copy link
Collaborator Author

Hmm. If we can assume that it can’t raise I can make a much smaller PR.

@iritkatriel
Copy link
Collaborator Author

On the other hand, it’s probably better to have the correct scopes for the try blocks (assuming it can raise, which handler is the right one?). This will help keep the code correct if we change it in the future.

@iritkatriel iritkatriel moved this from In Review to Done in Fancy CPython Board Jun 10, 2022
@iritkatriel iritkatriel moved this from Done to In Review in Fancy CPython Board Jun 10, 2022
@iritkatriel iritkatriel moved this from In Review to Done in Fancy CPython Board Jun 10, 2022
@markshannon
Copy link
Member

python/cpython#93622 is merged. Can we close this?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants