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

[postponed] Set f_trace_lines = 0/False on ignored frames #791

Closed
wants to merge 2 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Mar 31, 2019

This is meant for optimization, so that the trace handler is not called
for lines in blocks that are not traced.

TODO:

This is meant for optimization, so that the trace handler is not called
for lines in blocks that are not traced.
@blueyed
Copy link
Contributor Author

blueyed commented Mar 31, 2019

This is not much of a gain for the CTracer, but for PyTracer.

With this patch tox -e py37-coverage -- testing/test_asser* in pytest's repo takes 50.5s, without it 116.7s (using timid = 1 to use the PyTracer).

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
Copy link
Contributor Author

blueyed commented Mar 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
Copy link
Contributor Author

blueyed commented Mar 31, 2019

Added to cpython in python/cpython@5a85167 (bpo-31344).

There also f_trace_opcodes was added (which must be enabled explicitly).

@codecov-io
Copy link

codecov-io commented Mar 31, 2019

Codecov Report

Merging #791 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
coverage/pytracer.py 21.15% <33.33%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 820b255...6682675. Read the comment docs.

@nedbat
Copy link
Owner

nedbat commented Mar 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/issue11992 sigh

@blueyed
Copy link
Contributor Author

blueyed commented Mar 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 data_stack etc, to match the behavior of "return" not being called anymore).

@blueyed
Copy link
Contributor Author

blueyed commented Mar 31, 2019

pypy2 failure (https://travis-ci.com/nedbat/coveragepy/jobs/189126179) might be unrelated?!

@nedbat
Copy link
Owner

nedbat commented Apr 2, 2019

I tried restarting the pypy2 failure: https://travis-ci.com/nedbat/coveragepy/jobs/189126179 (oops, same URL...)

@@ -15,6 +15,9 @@
YIELD_VALUE = chr(YIELD_VALUE)


HAS_F_TRACE_LINES = sys.version_info >= (3, 7)

Copy link
Owner

@nedbat nedbat Apr 2, 2019

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 using hasattr

Copy link
Contributor Author

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?

Copy link
Owner

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?

Copy link
Contributor Author

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
Copy link
Owner

nedbat commented Apr 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
Copy link
Contributor Author

blueyed commented Apr 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 pbd.set_trace, yes.
It affects coverage when a debugger is used during test runs interactively, but is also an issue when you're testing with pdb itself (as with pdb++).

@nedbat
Copy link
Owner

nedbat commented Apr 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
Copy link
Contributor Author

blueyed commented Apr 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 None in the end.

@blueyed
Copy link
Contributor Author

blueyed commented Apr 7, 2019

Oh, sorry - I forgot about #791 (comment) / python/cpython#12640.
Yes, it should not get merged yet.

@cfbolz
Copy link
Contributor

cfbolz commented May 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?

@nedbat
Copy link
Owner

nedbat commented Jun 3, 2022

This same change was made in #1381

@nedbat nedbat closed this Jun 3, 2022
@blueyed blueyed deleted the f_trace_lines branch July 14, 2022 18:37
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

Successfully merging this pull request may close these issues.

4 participants