From 150813b232d06d08fb1096021a554812507e8b09 Mon Sep 17 00:00:00 2001 From: Juan Altmayer Pizzorno Date: Mon, 21 Aug 2023 19:28:27 -0400 Subject: [PATCH] - removed stats support to simplify upcoming changes, and given that they don't really seem useful (anymore); --- src/probe.cxx | 25 +------------ src/slipcover/__main__.py | 3 +- src/slipcover/bytecode.py | 29 --------------- src/slipcover/slipcover.py | 59 +---------------------------- tests/slipcover_test.py | 76 ++++++++++++-------------------------- 5 files changed, 28 insertions(+), 164 deletions(-) diff --git a/src/probe.cxx b/src/probe.cxx index f8f150c..4f66fa5 100644 --- a/src/probe.cxx +++ b/src/probe.cxx @@ -17,8 +17,6 @@ class Probe { bool _signalled; bool _removed; int _d_miss_count; - int _u_miss_count; - int _no_signal_count; int _d_miss_threshold; std::byte* _code; @@ -27,7 +25,7 @@ class Probe { _sci(PyPtr<>::borrowed(sci)), _filename(PyPtr<>::borrowed(filename)), _lineno_or_branch(PyPtr<>::borrowed(lineno_or_branch)), _signalled(false), _removed(false), - _d_miss_count(-1), _u_miss_count(0), _no_signal_count(0), + _d_miss_count(-1), _d_miss_threshold(PyLong_AsLong(d_miss_threshold)), _code(nullptr) {} @@ -84,19 +82,13 @@ class Probe { } } else { - ++_u_miss_count; + // U miss } Py_RETURN_NONE; } - PyObject* no_signal() { - ++_no_signal_count; - Py_RETURN_NONE; - } - - PyObject* mark_removed() { _removed = true; Py_RETURN_NONE; @@ -109,15 +101,6 @@ class Probe { Py_RETURN_FALSE; } - PyObject* get_stats() { - PyPtr<> d_miss_count = PyLong_FromLong(std::max(_d_miss_count, 0)); - PyPtr<> u_miss_count = PyLong_FromLong(_u_miss_count); - PyPtr<> total_count = PyLong_FromLong(1 + _d_miss_count + _u_miss_count + _no_signal_count); - return PyTuple_Pack(5, (PyObject*)_filename, (PyObject*)_lineno_or_branch, - (PyObject*)d_miss_count, (PyObject*)u_miss_count, - (PyObject*)total_count); - } - PyObject* set_immediate(PyObject* code_bytes, PyObject* offset) { #ifdef PYPY_VERSION PyErr_SetString(PyExc_Exception, "Error: set_immediate does not work with PyPy"); @@ -168,20 +151,16 @@ probe_set_immediate(PyObject* self, PyObject* const* args, Py_ssize_t nargs) { } METHOD_WRAPPER(signal); -METHOD_WRAPPER(no_signal); METHOD_WRAPPER(mark_removed); METHOD_WRAPPER(was_removed); -METHOD_WRAPPER(get_stats); static PyMethodDef methods[] = { {"new", (PyCFunction)probe_new, METH_FASTCALL, "creates a new probe"}, {"set_immediate", (PyCFunction)probe_set_immediate, METH_FASTCALL, "sets up for immediate removal"}, {"signal", (PyCFunction)probe_signal, METH_FASTCALL, "signals this probe's line or branch was reached"}, - {"no_signal", (PyCFunction)probe_no_signal, METH_FASTCALL, "like signal, but called only after this probe is removed"}, {"mark_removed", (PyCFunction)probe_mark_removed, METH_FASTCALL, "marks a probe removed (de-instrumented)"}, {"was_removed", (PyCFunction)probe_was_removed, METH_FASTCALL, "returns whether probe was removed"}, - {"get_stats", (PyCFunction)probe_get_stats, METH_FASTCALL, "returns probe stats"}, {NULL, NULL, 0, NULL} }; diff --git a/src/slipcover/__main__.py b/src/slipcover/__main__.py index ff80cc7..11e003f 100644 --- a/src/slipcover/__main__.py +++ b/src/slipcover/__main__.py @@ -35,7 +35,6 @@ # intended for slipcover development only ap.add_argument('--silent', action='store_true', help=argparse.SUPPRESS) ap.add_argument('--dis', action='store_true', help=argparse.SUPPRESS) -ap.add_argument('--stats', action='store_true', help=argparse.SUPPRESS) ap.add_argument('--debug', action='store_true', help=argparse.SUPPRESS) ap.add_argument('--dont-wrap-pytest', action='store_true', help=argparse.SUPPRESS) @@ -68,7 +67,7 @@ file_matcher.addOmit(o) -sci = sc.Slipcover(collect_stats=args.stats, immediate=args.immediate, +sci = sc.Slipcover(immediate=args.immediate, d_miss_threshold=args.threshold, branch=args.branch, skip_covered=args.skip_covered, disassemble=args.dis) diff --git a/src/slipcover/bytecode.py b/src/slipcover/bytecode.py index f842014..ef98704 100644 --- a/src/slipcover/bytecode.py +++ b/src/slipcover/bytecode.py @@ -590,35 +590,6 @@ def disable_inserted_function(self, offset): self.patch[offset] = op_JUMP_FORWARD - def replace_inserted_function(self, offset, new_func_index): - """Replaces an inserted function by another function.""" - - assert not self.finished - - if self.patch is None: - self.patch = bytearray(self.orig_code.co_code) - - assert self.patch[offset] == op_NOP - - it = iter(unpack_opargs(self.patch[offset:])) - next(it) # NOP - op_offset, op_len, op, op_arg = next(it) - if op == op_PUSH_NULL: - op_offset, op_len, op, op_arg = next(it) - - assert op == op_LOAD_CONST - - # FIXME to create a same-length replacement, we need to use the same number - # of EXTENDED_ARG opcodes used to encode op_arg; but arg_ext_needed gives us - # how many are needed at a minimum, not how many were actually used. - # Usually these are the same, but it's possible that unnecessary ones were used. - replacement = opcode_arg(op, new_func_index, arg_ext_needed(op_arg)) - assert len(replacement) == op_len - - op_offset += offset # we unpacked from co.co_code[offset:] - self.patch[op_offset:op_offset+op_len] = replacement - - def replace_global_with_const(self, global_name, const_index): """Replaces a global name lookup by a constant load.""" assert not self.finished diff --git a/src/slipcover/slipcover.py b/src/slipcover/slipcover.py index 13a59cb..f7a2ad2 100644 --- a/src/slipcover/slipcover.py +++ b/src/slipcover/slipcover.py @@ -34,10 +34,9 @@ def simplify(self, path : str) -> str: class Slipcover: - def __init__(self, collect_stats: bool = False, immediate: bool = False, + def __init__(self, immediate: bool = False, d_miss_threshold: int = 50, branch: bool = False, skip_covered: bool = False, disassemble: bool = False): - self.collect_stats = collect_stats self.immediate = immediate self.d_miss_threshold = d_miss_threshold self.branch = branch @@ -65,7 +64,6 @@ def __init__(self, collect_stats: bool = False, immediate: bool = False, self._get_newly_seen() self.modules = [] - self.all_probes = [] def _get_newly_seen(self): """Returns the current set of ``new'' lines, leaving a new container in place.""" @@ -102,7 +100,6 @@ def instrument(self, co: types.CodeType, parent: types.CodeType = 0) -> types.Co if isinstance(c, types.CodeType): ed.set_const(i, self.instrument(c, co)) - ed.add_const(probe.no_signal) # used during de-instrumentation probe_signal_index = ed.add_const(probe.signal) off_list = list(dis.findlinestarts(co)) @@ -156,9 +153,6 @@ def instrument(self, co: types.CodeType, parent: types.CodeType = 0) -> types.Co if self.disassemble: dis.dis(new_code) - if self.collect_stats: - self.all_probes.extend(probes) - if self.immediate: for tr, off in zip(probes, ed.get_inserts()): probe.set_immediate(tr, new_code.co_code, off) @@ -210,15 +204,7 @@ def deinstrument(self, co, lines: set) -> types.CodeType: func_index, func_arg_index, *_ = func if co_consts[func_index] == probe.signal: probe.mark_removed(co_consts[func_arg_index]) - - if self.collect_stats: - # If collecting stats, rather than disabling the probe, we switch to - # calling the 'probe.no_signal' function on it (which we conveniently added - # to the consts before probe.signal, during instrumentation), so that - # we have the total execution count needed for the reports. - ed.replace_inserted_function(offset, func_index-1) - else: - ed.disable_inserted_function(offset) + ed.disable_inserted_function(offset) new_code = ed.finish() if new_code is co: @@ -249,16 +235,6 @@ def get_coverage(self): simp = PathSimplifier() - if self.collect_stats: - d_misses = defaultdict(Counter) - u_misses = defaultdict(Counter) - totals = defaultdict(Counter) - for p in self.all_probes: - filename, lineno, d_miss_count, u_miss_count, total_count = probe.get_stats(p) - if d_miss_count: d_misses[filename].update({lineno: d_miss_count}) - if u_miss_count: u_misses[filename].update({lineno: u_miss_count}) - totals[filename].update({lineno: total_count}) - files = dict() for f, f_code_lines in self.code_lines.items(): if f in self.all_seen: @@ -292,21 +268,6 @@ def get_coverage(self): # the check for den == 0 is just defensive programming... there's always at least 1 line summary['percent_covered'] = 100.0 if den == 0 else 100*nom/den - if self.collect_stats: - # Once a line reports in, it's available for deinstrumentation. - # Each time it reports in after that, we consider it a miss (like a cache miss). - # We differentiate between (de-instrument) "D misses", where a line - # reports in after it _could_ have been de-instrumented and (use) "U misses" - # and where a line reports in after it _has_ been de-instrumented, but - # didn't use the code object where it's deinstrumented. - f_files['stats'] = { - 'd_misses_pct': round(d_misses[f].total()/totals[f].total()*100, 1), - 'u_misses_pct': round(u_misses[f].total()/totals[f].total()*100, 1), - 'top_d_misses': [f"{it[0]}:{it[1]}" for it in d_misses[f].most_common(5)], - 'top_u_misses': [f"{it[0]}:{it[1]}" for it in u_misses[f].most_common(5)], - 'top_lines': [f"{it[0]}:{it[1]}" for it in totals[f].most_common(5)], - } - files[simp.simplify(f)] = f_files summary = { @@ -407,22 +368,6 @@ def table(files): maxcolwidths = [None] * (len(headers)-1) + [missing_width] print(tabulate(table(cov['files']), headers=headers, maxcolwidths=maxcolwidths), file=outfile) - def stats_table(files): - for f, f_info in sorted(files.items()): - stats = f_info['stats'] - - yield (f, stats['d_misses_pct'], stats['u_misses_pct'], - " ".join(stats['top_d_misses'][:4]), - " ".join(stats['top_u_misses'][:4]), - " ".join(stats['top_lines'][:4]) - ) - - if self.collect_stats: - print("\n", file=outfile) - print(tabulate(stats_table(cov['files']), - headers=["File", "D miss%", "U miss%", "Top D", "Top U", "Top lines"]), - file=outfile) - @staticmethod def find_functions(items, visited : set): diff --git a/tests/slipcover_test.py b/tests/slipcover_test.py index 7b77c9c..fdcd10c 100644 --- a/tests/slipcover_test.py +++ b/tests/slipcover_test.py @@ -29,11 +29,10 @@ def ast_parse(s): return ast.parse(inspect.cleandoc(s)) -@pytest.mark.parametrize("stats", [False, True]) -def test_probe_signal(stats): +def test_probe_signal(): from slipcover import probe - sci = sc.Slipcover(collect_stats=stats) + sci = sc.Slipcover() t_123 = probe.new(sci, "/foo/bar.py", 123, -1) probe.signal(t_123) @@ -53,17 +52,11 @@ def test_probe_signal(stats): assert [123] == sorted(list(d["/foo/bar.py"])) assert [42, 314] == sorted(list(d["/foo2/baz.py"])) - assert ("/foo2/baz.py", 42, 1, 0, 2) == probe.get_stats(t_42) - assert ("/foo2/baz.py", 314, 0, 0, 1) == probe.get_stats(t_314) - - assert ("/foo/beast.py", 666, 0, 0, 0) == probe.get_stats(t_666) - -@pytest.mark.parametrize("stats", [False, True]) -def test_probe_deinstrument(stats): +def test_probe_deinstrument(): from slipcover import probe - sci = sc.Slipcover(collect_stats=stats) + sci = sc.Slipcover() t = probe.new(sci, "/foo/bar.py", 123, 3) probe.signal(t) @@ -77,15 +70,9 @@ def test_probe_deinstrument(stats): probe.mark_removed(t) # fake it since sci didn't instrument it probe.signal(t) # u-miss - probe.no_signal(t) - assert [] == sorted(sci.newly_seen.keys()) assert ["/foo/bar.py"] == sorted(sci.all_seen.keys()) - assert ("/foo/bar.py", 123, 3, 1, 6) == probe.get_stats(t) - - - def test_pathsimplifier_not_relative(): from pathlib import Path @@ -132,9 +119,8 @@ def check_line_probes(code): check_line_probes(const) -@pytest.mark.parametrize("stats", [False, True]) -def test_instrument(stats): - sci = sc.Slipcover(collect_stats=stats) +def test_instrument(): + sci = sc.Slipcover() base_line = current_line() def foo(n): #1 @@ -167,9 +153,8 @@ def foo(n): #1 assert [3] == [l-base_line for l in cov['missing_lines']] -@pytest.mark.parametrize("stats", [False, True]) -def test_instrument_generators(stats): - sci = sc.Slipcover(collect_stats=stats) +def test_instrument_generators(): + sci = sc.Slipcover() base_line = current_line() def foo(n): @@ -208,9 +193,8 @@ def foo(n): assert [] == cov['missing_lines'] -@pytest.mark.parametrize("stats", [False, True]) -def test_instrument_exception(stats): - sci = sc.Slipcover(collect_stats=stats) +def test_instrument_exception(): + sci = sc.Slipcover() base_line = current_line() def foo(n): #1 @@ -601,8 +585,7 @@ def test_instrument_long_jump(N): assert [] == cov['missing_lines'] -@pytest.mark.parametrize("stats", [False, True]) -def test_deinstrument(stats): +def test_deinstrument(): base_line = current_line() def foo(n): def bar(n): @@ -613,7 +596,7 @@ def bar(n): return x last_line = current_line() - sci = sc.Slipcover(collect_stats=stats) + sci = sc.Slipcover() assert not sci.get_coverage()['files'].keys() sci.instrument(foo) @@ -624,8 +607,7 @@ def bar(n): @pytest.mark.skipif(platform.python_implementation() == 'PyPy', reason="Immediate de-instrumentation does not work with PyPy") -@pytest.mark.parametrize("stats", [False, True]) -def test_deinstrument_immediately(stats): +def test_deinstrument_immediately(): base_line = current_line() def foo(n): def bar(n): @@ -636,7 +618,7 @@ def bar(n): return x last_line = current_line() - sci = sc.Slipcover(collect_stats=stats, immediate=True) + sci = sc.Slipcover(immediate=True) assert not sci.get_coverage()['files'].keys() sci.instrument(foo) @@ -650,9 +632,8 @@ def bar(n): assert foo.__code__.co_code[off] == bc.op_JUMP_FORWARD -@pytest.mark.parametrize("stats", [False, True]) -def test_deinstrument_with_many_consts(stats): - sci = sc.Slipcover(collect_stats=stats) +def test_deinstrument_with_many_consts(): + sci = sc.Slipcover() N = 1024 src = 'x=0\n' + ''.join([f'x = {i}\n' for i in range(1, N)]) @@ -676,9 +657,8 @@ def test_deinstrument_with_many_consts(stats): assert [*range(1,N)] == cov['missing_lines'] -@pytest.mark.parametrize("stats", [False, True]) -def test_deinstrument_some(stats): - sci = sc.Slipcover(collect_stats=stats) +def test_deinstrument_some(): + sci = sc.Slipcover() base_line = current_line() def foo(n): @@ -703,7 +683,7 @@ def foo(n): @pytest.mark.parametrize("do_branch", [False, True]) def test_deinstrument_seen_upon_d_miss_threshold(do_branch): - from slipcover import probe as tr + from slipcover import probe as pr t = ast_parse(""" def foo(n): @@ -727,12 +707,7 @@ def foo(n): foo(0) assert old_code != foo.__code__, "Code never de-instrumented" - - # skip probes with d_miss == 0, as these may not have been de-instrumented - probes = [t for t in old_code.co_consts if type(t).__name__ == 'PyCapsule' and tr.get_stats(t)[2] > 0] - assert len(probes) > 0 - for t in probes: - assert tr.was_removed(t), f"probe still instrumented: {tr.get_stats(t)}" + assert sum(pr.was_removed(t) for t in old_code.co_consts if type(t).__name__ == 'PyCapsule') > 0 cov = sci.get_coverage()['files']['foo'] if PYTHON_VERSION >= (3,11): @@ -863,9 +838,8 @@ def test_format_missing(): assert "2, 4" == fm([2,4], [1,3,5], [(2,3), (3,4)]) -@pytest.mark.parametrize("stats", [False, True]) -def test_print_coverage(stats, capsys): - sci = sc.Slipcover(collect_stats=stats) +def test_print_coverage(capsys): + sci = sc.Slipcover() base_line = current_line() def foo(n): @@ -891,9 +865,6 @@ def foo(n): output = output.splitlines() assert re.match(f'^tests[/\\\\]slipcover_test\\.py + {total} + {missd} +{round(100*execd/total)} +' + str(base_line+3), output[3]) - if stats: - assert re.match('^tests[/\\\\]slipcover_test\\.py +[\\d.]+ +0', output[8]) - def test_print_coverage_branch(capsys): t = ast_parse(""" @@ -934,9 +905,8 @@ def foo(x): assert re.match(f'^foo\\.py +{total_l} +{miss_l} +{total_b} +{miss_b} +{pct}', output[3]) -@pytest.mark.parametrize("do_stats", [False, True]) @pytest.mark.parametrize("do_branch", [True, False]) -def test_print_coverage_zero_lines(do_branch, do_stats, capsys): +def test_print_coverage_zero_lines(do_branch, capsys): t = ast_parse("") if do_branch: t = br.preinstrument(t)