Conversation
CodSpeed Performance ReportMerging #12232 will improve performances by 91.13%Comparing Summary
Benchmarks breakdown
|
|
The performance regression in the await benchmark appears to be noise, I can't reproduce it locally (but can reproduce some level of noisiness). And it doesn't use |
There was a problem hiding this comment.
Thanks for working on this. I read the discussion from #12230
It looks to me, but I would link this to the Deferred.debug flag.
I have never used capureVars in my applications and I don't know why the caputreVars functionality is usedful ...
Maybe captureVars is super important.
I hope @glyph will have some time to review this
My suggestion is to link this to the Deferred.debug flag and not with captureVars
src/twisted/python/failure.py
Outdated
| del d["__builtins__"] | ||
| localz = localz.items() | ||
| globalz = globalz.items() | ||
| while captureVars and f: |
There was a problem hiding this comment.
can we have this linked to debug ?
I did a quick search for caputeVars in src/twisted and I can see that caputeVars is only used in src/twisted/internet/defer.py and always linked to Deferred.debug
There was a problem hiding this comment.
Because captureVars is set by Deferred.debug, that's implicitly what's going on here already.
There was a problem hiding this comment.
Thanks. It looks like the captureVars argument is kind of a private argument, designed to support Deferred.debug.
All good.
adiroiban
left a comment
There was a problem hiding this comment.
Based on the discussion from #12230 it looks like Glyph is also happy with removing this functionalty.
I think that this PR looks good.
I don't think there is a need for any backward compatibitly policy, as I don't think that the content of the traceback is part of our public API.
Thanks for working on this!
|
The current code includes the stack optionally; the discussion in the issue suggests that removing it altogether is acceptable, so I'm going to update this to do that. |
…-location-exception-was-caught-in-failure
|
OK, the code just removes the |
adiroiban
left a comment
There was a problem hiding this comment.
Thanks for the cleanup.
I think that as part of this PR we should deprecate it.
That is:
- Remove it from the documentation, or document that it's deprecated.
- Raise deprecation warning when used.
- Add minimal tests for deprecation warnings.
Let me know if you need help with the testing.
Thanks again.
|
I haven't specifically tested, but this change would probably resolve some of the outstanding questions for #12092. If we don't need to include the additional traceback, then the changes it makes to the expected test output are fine. |
…-location-exception-was-caught-in-failure
adiroiban
left a comment
There was a problem hiding this comment.
Change looks good. Thanks.
I think that we need to use the incremental NEXT marker for version deprectation.
I have added it as needs changes, to prevent an accidental merge.
I am not sure if we will have an 24.8.0 release :)
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
|
So sounds like I can merge once tests passed, given change to NEXT? |
adiroiban
left a comment
There was a problem hiding this comment.
Thanks for the update. This can be merged.
…-caught-in-failure
…-caught-in-failure
Scope and purpose
Fixes #12230
See the issue for motivation, and a potential argument against accepting this PR.
The performance impact somewhat overlaps with #12231 but they both contribute if both are applied.