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

Ingest coverage data only for the files in the filter that actually have some #877

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

dvdoug
Copy link
Contributor

@dvdoug dvdoug commented Oct 27, 2021

For #876. The Psalm failure is bogus, its not even on a changed line!

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #877 (1ae01b6) into 9.2 (3ca2f86) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                9.2     #877      +/-   ##
============================================
- Coverage     83.10%   83.03%   -0.07%     
- Complexity     1116     1117       +1     
============================================
  Files            62       62              
  Lines          3652     3655       +3     
============================================
  Hits           3035     3035              
- Misses          617      620       +3     
Impacted Files Coverage Δ
src/Driver/PcovDriver.php 44.44% <0.00%> (-8.89%) ⬇️

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 3ca2f86...1ae01b6. Read the comment docs.

@dvdoug dvdoug marked this pull request as draft October 27, 2021 18:25
@dvdoug dvdoug force-pushed the optimise_pcov branch 2 times, most recently from deb3d57 to ac4309d Compare October 27, 2021 18:32
@dvdoug dvdoug marked this pull request as ready for review October 27, 2021 18:33
@sebastianbergmann
Copy link
Owner

@krakjoe You wanted a ping when the PCOV driver is changed.

@sebastianbergmann
Copy link
Owner

@indreka Can you please test whether this patch improved PCOV driver performance for you? Thanks!

@krakjoe
Copy link
Contributor

krakjoe commented Oct 28, 2021

There is no sense in entering collect() or calling clear(), if there is nothing returned from waiting()

@indreka
Copy link

indreka commented Oct 28, 2021

@sebastianbergmann I can confirm that the proposed change works very well!
Output of coverage.xml is same when I compared my originally proposed "hack" vs proposed solution.
In addition the speed has also improved, subset of testsuite with my "hack" locally runs 55sec, the proposed solution makes it run with 35sec! Will add custom patching to circleci runner also to verify final speedup.

@sebastianbergmann
Copy link
Owner

There is no sense in entering collect() or calling clear(), if there is nothing returned from waiting()

Thank you for your feedback. However, I am not sure whether the patch changes the code to do what you propose (in which I case I would accept it). Might be because I have a hard time understanding the line of code in question due the use of the ternary operator.

@dvdoug Could you please change the code to not use a ternary? Thanks!

@krakjoe
Copy link
Contributor

krakjoe commented Oct 28, 2021

However, I am not sure whether the patch changes the code to do what you propose

I'm not sure if that was directed at me.

For the avoidance of any doubt: Do not call collect unconditionally. Do not call clear if nothing was returned from waiting().

This is for performance reasons, and this code is extremely perf sensitive; It cannot make unnecessary calls.

@sebastianbergmann
Copy link
Owner

I'm not sure if that was directed at me.

It was, sorry for not being clear about this.

For the avoidance of any doubt: Do not call collect unconditionally. Do not call clear if nothing was returned from waiting().

This is for performance reasons, and this code is extremely perf sensitive; It cannot make unnecessary calls.

@dvdoug Can you incorporate Joe's suggestions? Thanks!

@indreka
Copy link

indreka commented Oct 28, 2021

Can confirm that in circleci this change seems to make coverage scan ~5% faster when comparing to previous phpunit 8.5 + pcov setup we had

@dvdoug
Copy link
Contributor Author

dvdoug commented Oct 28, 2021

For the avoidance of any doubt: Do not call collect unconditionally. Do not call clear if nothing was returned from waiting().

Happy to reorganise things to do this, but if a testcase runs without managing to cover a single line of relevant source code then I'd honestly be pretty surprised. I'd have thought that the overhead of including an additional if here in userland would be slower than an occasional extra call into the extension during a zero-coverage scenario.

@sebastianbergmann sebastianbergmann merged commit bb472a1 into sebastianbergmann:9.2 Oct 29, 2021
@sebastianbergmann
Copy link
Owner

Thank you, @dvdoug!

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