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

Lazily fill in __context__ during unwinding #487

Closed
markshannon opened this issue Oct 31, 2022 · 7 comments
Closed

Lazily fill in __context__ during unwinding #487

markshannon opened this issue Oct 31, 2022 · 7 comments

Comments

@markshannon
Copy link
Member

When a new Exception object is created, its __context__ is set to the "handled exception"
Unfortunately the "handled exception" cannot be determined at compile time or locally, and requires a scan of the call stack.

Rather than perform this expensive check, we could add __context__s during unwinding.
The code to do so could be generated by the compiler. We already emit cleanup code in except: blocks, this would be a bit more cleanup.
Doing so would speed up EAFP error handling, skipping the expensive step of setting up the __context__.
This is semantic change, not just a performance improvement, so will need a wider discussion if we decide it is worthwhile.
IMO, this is an improvement to the semantics as the __context__ only makes sense once the scope of that context has been unwound.

Removing the need to scan the stack would also speedup generators and coroutines as we would no longer need to maintain the stack of exceptions to scan.

@iritkatriel Does this make sense to you?

@iritkatriel
Copy link
Collaborator

The stack is only scanned to skip over non-Null/Nones, so maybe that can be optimised without changing semantics, simply creating another link to the "next non-null/None exc"?

@markshannon
Copy link
Member Author

That would complicate pushing and popping generator frames, as we would need to check whether the current exception is NULL. It would also require adding the generator frame on entry to a handler, and removing it again on exit.
Probably too inefficient, and error prone.

@iritkatriel
Copy link
Collaborator

IMO, this is an improvement to the semantics as the __context__ only makes sense once the scope of that context has been unwound.

I agree, and I think this would solve this problem: python/cpython#63061

@iritkatriel
Copy link
Collaborator

On the other hand, I don't know how setting it during unwinding would work in this case:

def f():
  raise ValueError(42)

try:
   f()
except:
   try:
      raise TypeError(1)
   except:
      raise TypeError(2)

The outer ValueError is the context of the TypeError(1), but at the time of unwinding we are seeing The ValueError and the TypeError(2) (whose context is TypeError(1), but there could also be a cause messing things up further).

@iritkatriel
Copy link
Collaborator

I think it was a mistake to add both __cause__ and __context__ to exceptions. Instead of making the exception chain a tree, it would have been better to have one __chained__ exception + a flag to indicate whether it is the cause or context. I don't know if there are use cases where you really need both. The tree is causing complexity and unsolvable edge cases.

Not sure about the prospects of changing this now.

@ericsnowcurrently
Copy link
Collaborator

CC @benjaminp

@markshannon
Copy link
Member Author

I think this is too complex to be worthwhile. Any performance gains would be slight and a simpler approach will get some of the benefit for much less effort.

@markshannon markshannon closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants