Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork464
[postponed] Set f_trace_lines = 0/False on ignored frames#791
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is meant for optimization, so that the trace handler is not calledfor lines in blocks that are not traced.
blueyed commentedMar 31, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This is not much of a gain for the CTracer, but for PyTracer. With this patch CTracer: 20.5s without this patch, 20.0s with it (but just run once, it is likely due to a function call being less of an overhead in C). |
blueyed commentedMar 31, 2019
This appears to be py37+ only. And pdb/bdb would need to be fixed to set it to True explicitly (https://bugs.python.org/issue36494). |
blueyed commentedMar 31, 2019
Added to cpython inpython/cpython@5a85167 (bpo-31344). There also |
codecov-io commentedMar 31, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #791 +/- ##==========================================- Coverage 90.24% 90.22% -0.02%========================================== Files 78 78 Lines 10884 10887 +3 Branches 1127 1128 +1 ==========================================+ Hits 9822 9823 +1- Misses 939 941 +2 Partials 123 123
Continue to review full report at Codecov.
|
nedbat commentedMar 31, 2019
We should be returning None for the trace function if we don't want to trace the frame, and that should be enough. But I don't see where we're returning None. Or is this the decade-old bug? Yep, it is:https://bugs.python.org/issue11992sigh |
blueyed commentedMar 31, 2019
I've tried returning None also before, but stopped when noticing that several tests were failing due to it, and assumed coverage.py needs to trace everything still internally. (I've also tried (quickly) to not add to |
blueyed commentedMar 31, 2019
pypy2 failure (https://travis-ci.com/nedbat/coveragepy/jobs/189126179) might be unrelated?! |
nedbat commentedApr 2, 2019
I tried restarting the pypy2 failure:https://travis-ci.com/nedbat/coveragepy/jobs/189126179 (oops, same URL...) |
| HAS_F_TRACE_LINES=sys.version_info>= (3,7) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd rather add a constant to PYBEHAVIOR in env.py, perhaps usinghasattr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it require to get a frame object first? (i.e.sys._getframe()) to inspect it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, though that should be easy. Though come to think of it, why not just do the hasattr where you try to set the attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My idea was to have as less overhead as possible in the tracing function itself.
nedbat commentedApr 2, 2019
The bug about pdb: it affects people trying to stop in the debugger when using coverage? I guess that could happen if you are running a test suite, and want to break on failures? |
blueyed commentedApr 2, 2019
The problem/bug with pdb is "just" that coverage.py relies on its trace function being called, which is not the case anymore after |
nedbat commentedApr 7, 2019
@blueyed Are you saying we shouldn't merge this until pdb is changed? Or just that they will want to eventually change? |
blueyed commentedApr 7, 2019
The discussion above is not related to this PR really, which should just improve performance. I was replying to the "bug about pdb" - which is more a conceptual problem due to only having a single trace function only. pdb could restore any previous one (i.e. coverage.py's), instead of setting it to |
blueyed commentedApr 7, 2019
Oh, sorry - I forgot about#791 (comment) /python/cpython#12640. |
cfbolz commentedMay 23, 2022
I haven't dug super deep into this PR, but maybe it's easier to restart this now that coverage only supports 3.7 upwards? |
0c14f1a to82169a6Comparenedbat commentedJun 3, 2022
This same change was made in#1381 |
Uh oh!
There was an error while loading.Please reload this page.
This is meant for optimization, so that the trace handler is not called
for lines in blocks that are not traced.
TODO: