Conversation
47286b6 to
861c2d6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jbcoe-restore-operator-overload-set-guard #369 +/- ##
=============================================================================
+ Coverage 72.79% 73.47% +0.68%
=============================================================================
Files 12 15 +3
Lines 838 1003 +165
Branches 221 221
=============================================================================
+ Hits 610 737 +127
- Misses 22 60 +38
Partials 206 206
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
861c2d6 to
271d803
Compare
271d803 to
a831360
Compare
a7015cd to
ff104bb
Compare
|
This check takes ~35 mins to run. I'm not happy with that and am comfortable losing coverage checks on compile time code. @philipcraig WDYT? |
ff104bb to
67a29c8
Compare
I have a threading of this work idea -- stay tuned |
oh! looks like from your last commit you have the same idea |
philipcraig
left a comment
philipcraig
left a comment
There was a problem hiding this comment.
Second-round pass over the whole stack (#336 → #369). Multi-header instrumentation works, and the per-shard LCOV output only lists that shard's probe points, so the Codecov merge across the matrix is sound. Two things inline.
On the run time: the sharding here may well be enough, and #374 collects the further ideas from this review (keying the trap on header and line so there is one work directory and one verify pass, running the verify compiles through the pool, probing the cheapest translation unit first). Either route needs a look once the timings from this PR's matrix are in, so I will re-review that part later rather than now.
| probe_jobs: list[tuple[ProbeConfiguration, ProbePoint]] = [] | ||
| for header in shard_headers: | ||
| header_work_dir = os.path.join(arguments.work_dir, header) | ||
| os.makedirs(header_work_dir, exist_ok=True) |
There was a problem hiding this comment.
exist_ok=True only tolerates an existing directory. After a run on main, build/consteval-coverage/protocol.hh is a regular file written by the single-header layout, so the first run on this branch dies before any compile:
File "scripts/consteval_coverage.py", line 602, in main
FileExistsError: [Errno 17] File exists: '.../build/consteval-coverage/protocol.hh'
CI is unaffected (fresh checkout). A shutil.rmtree(arguments.work_dir, ignore_errors=True) before the loop fixes it.
|
|
||
| # Translation units that instantiate protocols and so drive the consteval | ||
| # machinery in protocol.hh. | ||
| # machinery in the instrumented headers. |
There was a problem hiding this comment.
name_mangling.h is now instrumented (74 of 177 probe points) but name_mangling_tests.cc is not a default translation unit, and it is the only TU that reaches signed_decimal, integral_decimal, mangle_template_argument_value and the is_value branch. Against the three defaults those probes never fire, so 11 lines that name_mangling_tests.cc does exercise (Tagged<3>, Tagged<-3>, Tagged64<4294967296LL>) land as DA:...,0 on Codecov. Adding it costs about two seconds per compile, and it is the cheapest TU, so probing it first also saves time.
|
Measured the shards: the probe step is 9.3–11.8 min per shard against 36.3 min unsharded, at four times the runner minutes. Each shard still repeats 18–21 serial verify compiles, because round-robin sharding puts every header in every shard. Let's land this as is, with the two inline fixes, and take the serial and per-probe costs in #374 afterwards. The numbers are on that issue. |
b35abdb to
6ccfb5c
Compare
67a29c8 to
b306f68
Compare
consteval_coverage.py only instrumented protocol.hh, which after the detail.hh split keeps 3 of the ~21 original consteval sites; the rest moved to conformance.hh, vtable.hh, member_function_thunks.hh, operator_thunks.hh, protocol_wrappers.hh and name_mangling.h. The coverage job was reporting a percentage of almost nothing. Instrument all seven headers, tagging each trap with a file index alongside the line number since line numbers collide across headers. The disarmed sentinel is a single NO_PROBE_ARMED constant reused by both the generated C++ macro defaults and the Python call sites, rather than -1 hardcoded independently in three places. ProbePoint carries its file_index directly instead of recomputing it via INSTRUMENTED_HEADERS.index() in run_probe, and write_lcov groups probe_points by header once instead of re-filtering the full list per header. Addresses philipcraig's review comment on the #336-#355 stack (out of diff): consteval_coverage.py only instruments protocol.hh. Fix header instrumentation not reaching transitively-included headers Per-header work dirs only copied the target header's instrumented copy; every other instrumented header still lived in SOURCE_ROOT, so a header reached only transitively (e.g. name_mangling.h via protocol.hh) resolved back to its uninstrumented original and was silently reported as 0% covered. Each header's work dir now also gets plain copies of every other instrumented header, so quote-includes among them stay local. Updates the stale protocol.hh-only doc comments this broke. Shard consteval-coverage probing across a CI matrix Compiling every probe point once per header, single-threaded on a 4-core runner, takes ~38 minutes once all 21 consteval sites are probed instead of the original 3. Adds --shard-count/--shard-index to consteval_coverage.py, round-robining probe points (not whole headers, whose sizes vary too much to balance) across shards, and wires up a 4-way matrix in coverage.yml.
b306f68 to
df01d8c
Compare
|
This is too slow and @philipcraig has a better PR Closing.. |
Closes #374. Replaces one compile per probe point with batched rounds, and instruments every library header instead of just `protocol.hh`. ## Timings The CI probe step for the same 177 probe points, single runner unless stated: | Run | Probe points | Probe step | |---|---|---| | main (`protocol.hh` only) | 83 | 10.1 min | | #369 unsharded | 177 | 36.3 min | | #369 four shards | 44 each | 9.3–11.8 min per shard, 4× runner minutes | | this PR | 177 | 3.1 min | The whole job now takes 5 minutes, and `timeout-minutes` drops from 90 to 30. Locally, against the one-compile-per-probe script from #369 with `name_mangling_tests.cc` added: | Script | Cores | Wall | Rounds | |---|---|---|---| | #369, one compile per probe | 24 | 419 s | – | | this PR | 4 | 94 s | 23 | | this PR, contiguous instead of interleaved batches | 4 | 155 s | 42 | Both scripts report 150/177 probe points covered, with identical per-header LCOV records. ## How - Traps are keyed on `(header index, __LINE__)`, so all headers are instrumented in one work directory and one `-include` file arms any subset of them. - Each round arms every probe point not yet covered and compiles a translation unit once per batch. GCC reports every independent constant evaluation that throws, so one compile finds many points; a point masked by an earlier trap on its path surfaces in the next round once that trap is disarmed. Rounds stop when one finds nothing new (from jbcoe's prototype on the closed #369 branch). - Batches are interleaved rather than contiguous, so neighbouring probe points, which usually share an evaluation path, do not mask each other. - Translation units are probed cheapest first (`name_mangling_tests.cc` is now included), so `protocol_test.cc` only ever arms what the cheaper ones left uncovered. - Unarmed traps are `if constexpr` no-ops, so they cost the constant evaluator nothing. - A compile that fails with no trap fired is reported as an instrumenter bug, which replaces the separate unarmed verify pass. - The CI job is back to a single runner.
2 participants
Fixes a point raised in philipcraig's review of the #336-#355 stack (out of diff on #355): consteval_coverage.py only instruments protocol.hh, which keeps 3 of 21 consteval sites after the detail.hh split, so the coverage job measures almost nothing.