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

Use a flat array for line coverage. (take 2) #15

Closed
wants to merge 2 commits into from

Conversation

taavi
Copy link
Contributor

@taavi taavi commented Feb 9, 2012

I've rebased and done some extra cleanup (amazing what 6 months and fresh eyes will do). I also ran benchmarks again, this time with public code!

Using PHP 5.3.10 x86_64 compiled on OSX 10.6.8, run on a 3.2GHz i3 with 8GB 1333MHz RAM.

Each timing is the mean of 10 runs of PHPUnit 3.6 (sebastianbergmann/phpunit@5d0ff52). Before each run of 10, I ran it 3 times in a row and threw out those results.

for x in {1..10}; do time ./phpunit.php 2>&1 >/dev/null; done

Before (derickr:master derickr/xdebug@004b99c):
real avg 18.414 s
user avg 15.76 s
sys avg 2.3133 s

After (taavi:coverage_line_array2 taavi/xdebug@cae8255):
real avg 17.666 s (4% improvement over before)
user avg 15.0101 s (5% improvement over before)
sys avg 2.2652 s (2% improvement over before)

I suspect it also reduces memory usage (given the cost of the hash tables and linked list entries vs 1-byte-per-line-in-a-file) and almost certainly reduces memory fragmentation (one alloc, and probably a few reallocs per file vs alloc-per-line).

Thanks!

Taavi Burns added 2 commits February 9, 2012 14:16
Remove unnecessary line break. Coverage line numbers start at 1.
@taavi
Copy link
Contributor Author

taavi commented Mar 13, 2012

Is this good to be merged in before the 2.2 release? Let me know if there's anything else I can update/explain to get it in. Thanks!

@derickr
Copy link
Contributor

derickr commented Mar 13, 2012

Would you terribly mind if we save it for 2.2.1? I'd like to get 2.2.0 out ASAP with minimal fuss. I've also added an issue in the issue tracker for this now: http://bugs.xdebug.org/view.php?id=792

@sebastianbergmann
Copy link
Contributor

+1 for releasing 2.2.0 without this and then have a look at it for 2.2.1.

@taavi
Copy link
Contributor Author

taavi commented Mar 13, 2012

Oh, absolutely! I didn't know 2.2 was already going to RC, and that this would be okay to get into 2.2.1. Just didn't want a patch-that-would-eventually-be-accepted to take years to get in. :) Thanks!

@FaKleiser
Copy link

Why does the xdebug_get_code_coverage() method only return whether a line was executed or not? Wouldn't it be better to return the execution count per line? This does not affect the statement coverage tools available, as knowing whether a line was executed at least once or zero times is enough information for that purpose.
However, having this information allows to compute a loop coverage (has every loop been executed zero times, once, and at least twice?) which could improve the quality of PHP web applications a lot!

I had a quick glance at the source code and changed line 654 of xdebug_code_coverage.c of the current revision to add_index_long(retval, line->lineno, line->count); which returns the internal execution count. The output is quite fancy, but concerning that loops and if-clauses commonly have more than one statement per line, not too unexpected.
Nevertheless, i can't really make use of the modification as i can't understand how the internal execution counts are computed. For example, the statement $r *= 2 in a single line returns a count of 2 when executing exactly once, but having $r = 2 returns a count of 3 under the same conditions!

All in all, it would be great to have the possibility to compute more than just statement coverage in PHP applications!

@derickr
Copy link
Contributor

derickr commented May 20, 2014

So, it has been taking years (my fault). Do you think this is still relevant?

As for the comment by @FaKeller, this is tracked in http://bugs.xdebug.org/view.php?id=1034

@FaKleiser
Copy link

Great, I would love to see branch/path coverage in xdebug! :-)

@derickr
Copy link
Contributor

derickr commented Oct 5, 2014

The branch/path coverage has landed now. That also means, that this patch no longer applies cleanly. As the branch/path coverage is brand spanking new, I would appreciate it if you have a look to see whether something can be improved in the new code. I think however, this PR should now be closed.

@derickr derickr closed this Oct 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants