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

Enhance coverage collection performance by caching probe arrays local… #545

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

jon-bell
Copy link
Contributor

@jon-bell jon-bell commented Dec 28, 2018

This PR significantly improves and simplifies the performance of coverage collection. This PR is related to #534, which also modifies coverage collection (sorry for the delay in putting this together, a few other things came up in the meantime).

The prior mechanism for collecting coverage involved hitting a HashMap at least once in each method execution to store coverage probes (there were several different ways that this might happen depending on the size of a method). This PR simplifies coverage collection so that there only is a single access to this shared HashMap (when a class is initialized). A static field is added to each class with a probe array, within each method this static field is cached in a local variable, and then when any probe is hit, this array is directly updated). The global HashMap keeps track of all of these class-local arrays, which are collected and reset when coverage is dumped.

This approach can yield a significant speedup in coverage collection. As an example (measured on my laptop, not very scientifically/controlled, but to demonstrate the order-of-magnitude improvement): running pit to collect coverage on apache commons-math took 161 seconds using version 1.4.3. In comparison, collecting coverage with JaCoCo (and dumping coverage after each test) took 70 seconds. Using this patched version of pit, the same exact coverage collection took only 73 seconds.

Here's a source-level example of the instrumentation result:

public class Foo {
   public static final int $$pitCoverageProbeSize = 10; //however many blocks there are + 1
   public static final byte[] $$pitCoverageProbes = CodeCoverageStore.getOrRegisterClassProbes(thisClassID,$$pitCoverageProbeSize);

   private void bar(){
     byte[] localRefToProbes = $$pitCoverageProbes;
     //line of code
     localRefToProbes[1] = 1; //assuming above line was probe 1
   }

 }

CodeCoverageStore maintains a reference to all of these $$pitCoverageProbes arrays
and empties them out between each test.

Here is an argument for thread safety:
If two threads are racing to update the probe array, there will be no incorrect behavior - this is an idempotent operation.

If one thread is racing to update the probe array while another is racing to read it and dump coverage, then it is safe to assume that there is already no happens-before relationship between that line of code running and the test ending (since we dump coverage when the test ends), and there’s nothing that we can do to solve that, regardless of how we lock. That is to say, if a line is deterministically covered by a test then we guarantee that we will report it as covered; if a line is not deterministically covered by a test, then we do not guarantee that we will report it as covered (but again, this was already the case).

@hcoles
Copy link
Owner

hcoles commented Jan 7, 2019

Thanks, it's going to be some time before I have chance to look at this but it is certainly something I'd like to merge in.

@jon-bell
Copy link
Contributor Author

jon-bell commented Jan 7, 2019

No problem - no rush. Thank you very much!

@hcoles hcoles merged commit dfc57a3 into hcoles:master Mar 26, 2019
@hcoles
Copy link
Owner

hcoles commented Mar 26, 2019

Finally got chance to look at this and couldn't see any obvious issues - thanks.

The plan is to release without changing the block definition, so any issues that might turn up once it is in use in real world projects can be quickly isolated to this change. If everything goes smoothly the block size will be reduced to consider method calls in the next release.

@jon-bell
Copy link
Contributor Author

Yay! Thanks! I am really, very happy to help - if you need me to rebase any of this stuff to a newer commit, or develop additional tests, please let me know.

@hcoles
Copy link
Owner

hcoles commented Mar 26, 2019

Thanks, that could certainly be helpful.

I've not had chance to properly go through the other PR yet (so the following is based on suspicion rather than anything concrete), but I suspect it could be made much smaller/simpler if only method calls are being considered as ending a block. Smaller changes are obviously easier to merge.

@jon-bell
Copy link
Contributor Author

I'll move this discussion over there.

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.

2 participants