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

Improve developer experience of code coverage performance improvements #3301

Closed
sebastianbergmann opened this issue Sep 11, 2018 · 33 comments
Closed

Comments

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

With #3272 and #3290, the performance of the collection and processing of code coverage data is significantly improved when Xdebug 2.6 (or later) is used.

Right now, this requires PHPUnit to be invoked with --dump-xdebug-filter to generate a PHP script with whitelist configuration code for Xdebug any time the whitelist configuration is changed in phpunit.xml. PHPUnit then needs to be invoked with --prepend to load that generated PHP script before any other code is loaded.

This ticket proposes the idea of removing the --dump-xdebug-filter and --prepend options (right now this would not be a BC break as, at this point, they were never released) and instead always generate a .phpunit.xdebug.filter PHP script in the directory in which PHPUnit is invoked (similar to .phpunit.result.cache) and automatically load this file if it exists and Xdebug > 2.6 is used.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

@sebastianheuer @hollodotme Do you think that this makes sense / could work?

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

@sebastianbergmann Makes absolutely sense to me and actually I'd prefer this proposed approach, because the current implementation would need updates on every CI config to invoke phpunit twice (per test suite).

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Sep 11, 2018

+1, but wouldn't this still require two runs of phpunit?

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

It would be slow on the first run and fast(er) on subsequent runs

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

We may need to store a hash of phpunit.xml in .phpunit.xdebug.filter and not load it when it's out of date.

@epdenouden
Copy link
Collaborator

@epdenouden epdenouden commented Sep 11, 2018

Keeping an eye on the configuration and figuring out if a cache is still relevant (or even valid) for a new configuration is something I ran into, too.

Things that popped up when working on the cache test reordering:

  • how much of a configuration-change ends up with a different test; in theory none at all, obviously, but in the real world when people use it as an integration testrunner, it gets messy
  • what to do on aborted runs: did something go so wrong that the new data is not safe to cache? did the user abort us on purpose, or did we just run out of disk/memory/etc?
  • related to previous, do you update the cache while running, effectively forcing you to use something like SQLite? Or just dump-on-exit which is simple, robust and fast.

I have some sketches for an improved cache that behaves much nicer in such edge cases.

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Sep 11, 2018

One of the issues I'm seeing (while implementing this) is caused by the requirement to call xdebug_set_filter() before any PHPUnit code is loaded - how could I get a hash of the configuration before PHPUnit is bootstrapped and I know which configuration file (if any) is actually used?

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

@sebastianheuer Good point. So my question is how much the overall execution slows down, if the .phpunit.xdebug.filter is dumped every single time based on the whitelist from the phpunit.xml and then phpunit invokes itself with the --prepend .phpunit.xdebug.filter option. Reading the config, dumping the whitelist filter and restarting PHPUnit shouldn't have so much impact on overall execution time, IMHO.

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Sep 11, 2018

Dumping the whitelist is pretty quick, it would add maybe 2 seconds to the overall execution time. I remember @sebastianbergmann and I were discussing this behaviour during the code sprint, but it felt a bit weird.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

@hollodotme One thing I do not want is to restart PHPUnit. The complexity this would add does not, IMO, is not justified by also having the first run be fast(er).

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

Yes, programs restarting themselves is kind of weird. Even though composer does that in some cases as far as I know. How about the idea to make that explicit for the user. Instead of phpunit --dump-xdebug-filter + phpunit --prepend .phpunit.xdebug.filter simply run phpunit --xdebug-filter which combines both commands. So users can choose. That would also allow to switch the whole feature off and quickly compare results between executions with Xdebug filter and executions without.

EDIT: @sebastianbergmann Sorry, didn't see your comment while posting.

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

@sebastianbergmann How would you have handled the second invocation regarding your initial proposal? Or did you mean that the first run dumps the file, but does not apply the Xdebug filter and is slower for that reason?

@keradus
Copy link
Contributor

@keradus keradus commented Sep 11, 2018

One thing I do not want is to restart PHPUnit. The complexity this would add (...)

@sebastianbergmann , https://github.com/composer/xdebug-handler may be interesting idea - not perfect match for this case, but it automate when and how CLI command shall be restarted

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

@hollodotme That is exactly what I mean.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

@keradus I know about xdebug-handler, but even just using that (without having to implement the functionality ourselves) adds too much complexity, IMO.

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Sep 11, 2018

Another approach could be to blacklist PHPUnit's own code first before bootstrapping PHPUnit and setting a whitelist for the code under test before loading the tests. A quick proof-of-concept proved to bring almost the same speed improvements as the prepended whitelist.

I do think that this approach would probably work best with the PHAR, as that contains all the dependencies, whereas only parts of PHPUnit would be blacklisted when installed via composer, as the project's vendor directory would not be part of the blacklist.

While this will not give us the maximum benefit, it would not require any changes or additional steps on the user side.

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

I like that approach @sebastianheuer!

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

Can blacklist and whitelist be used together?

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Sep 11, 2018

According to Xdebug's documentation not, but then again it seems to work ;) Without knowing the internals I can only assume that the whitelist replaces the blacklist, which is okay in this case as all the backlisted code has already been loaded at that point in time so that no coverage information is collected.

@hollodotme
Copy link

@hollodotme hollodotme commented Sep 11, 2018

@sebastianheuer I don't think it is actually used in combination in your implementation. I think the second xdebug_set_filter removes the blacklist and sets the whitelist which also excludes the paths that you had in the blacklist, I assume. IMO, to test this we need to check what happens to paths that are in both, black- and whitelist.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 11, 2018

I would like to get input from @derickr on this. However, he is currently on vacation AFAIK.

@derickr
Copy link
Contributor

@derickr derickr commented Sep 19, 2018

I think it's best to automatically restart PHPUnit, like Composer does, so that this benefits all users, even ones that have not heard of a cache at all.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 19, 2018

@sebastianheuer Can you implement this and send a pull request?

@sebastianbergmann sebastianbergmann removed this from the PHPUnit 7.4 milestone Sep 28, 2018
@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Sep 28, 2018

I am going to release what we have right now (what @sebastianheuer implemented during the last code sprint) next week as part of PHPUnit 7.4. We can improve on this later.

@sebastianheuer
Copy link
Contributor

@sebastianheuer sebastianheuer commented Oct 1, 2018

I am struggling a bit with implementing the composer-style restart. One of the simplest things I can imagine right now is calling passthrough() with the command coming from $argv. I got that working in a PoC, but it would require some smaller changes like omitting the version output in the second run - and I cannot really imagine the possible pitfalls there. Is this what you had in mind, @sebastianbergmann ?

@soullivaneuh
Copy link

@soullivaneuh soullivaneuh commented Jan 15, 2019

Adding my two cents: Talking about --dump-xdebug-filter, it would be great if the generated PHP script warn us when the xdebug_set_filter function does not exist.

Currently, it just silently does not a thing and we won't obviously check this part.

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Jan 15, 2019

@soullivaneuh There is not really something we can do about this (that makes sense), I am afraid.

If your Xdebug does not have xdebug_set_filter() then you are missing out on not just this feature but also on quite a few bug fixes. Just saying :-)

@zlikavac32
Copy link

@zlikavac32 zlikavac32 commented Jan 15, 2019

@sebastianbergmann Perhaps an early fork that generates file on a predefined location (before fork) which is later included?

It could be a clean solution, where fork just executes dump.

One step further could be to use shared memory + eval so that nothing is written do disc?

@gdsmith
Copy link

@gdsmith gdsmith commented Jan 17, 2019

Is there any comparison of this versus running phpunit through phpdbg which I've seen give some performance improvements.

Eg:

phpdbg -qrr vendor/bin/phpunit

(options quiet rrun and quit)

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Jan 17, 2019

phpdbg might be faster but in my experience the data it provides is less accurate that the data provided by Xdebug. Which is why I do not care about phpdbg at this point in time.

@gdsmith
Copy link

@gdsmith gdsmith commented Jan 17, 2019

That's interesting, thanks, do you have any more info on those accuracy issues?

@stale stale bot added the stale label Jul 2, 2019
Repository owner deleted a comment from stale bot Jul 2, 2019
@flobee
Copy link

@flobee flobee commented Sep 7, 2019

Hints:
DE: Habe gerade den Artikel erst gefunden (https://thephp.cc/news/2019/01/faster-code-coverage) but...

If you dont use composers autoload (opcache and other caches OFF) the improvement is low but improves :-)

Runtime: PHP 7.3.9-1+0~20190902.44+debian10~1.gbpf8534c with Xdebug 2.7.2
Phpunit via composer, not as phar.
Tests: 732, Assertions: 2539, Errors: 1, Skipped: 2. 

--> before >-- 
Generating code coverage report in HTML format ... done [2.38 seconds]
./runCoverageCreate.sh src/  29,17s user 1,21s system 82% cpu 36,999 total 

--> after >--
Generating code coverage report in HTML format ... done [1.99 seconds]
./runCoverageCreate.sh src/ --prepend build/xdebug-filter.php  28,45s user 1,01s system 81% cpu 36,341 total

Well. This seems to be the vendor/phpunit/* stuff

I would expect NOT implementing/ activate this improvment by default (or already in?). A flag for the phpunit.xml would be good!
Worst case senario is a default in my oppinion.
For the phpunit.xml config generator as option to request to enable it (and default to "Y")

Kind regards

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Feb 12, 2020

I do not think that there is any value in pursuing this further. Personally, I stopped using Xdebug for code coverage a while ago and instead use PCOV. Eventually we may even want to remove --dump-xdebug-filter as the implementation of --prepend is a hack (because it has to be).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants