Remove extra work when computing code coverage

[kageiit]

0cc8d5d introduced a performance degradation for projects that do not use the direct_to_jar spool mode when computing code coverage

Ideally, the classes dir can be used if it already exists to prevent unzipping for projects that output classes to the filesystem already.

[tdrhq]

@kageiit This should be easy to do, but how much of a regression are we talking about? The code coverage logic has broken a few times in the past, so having fewer variations on the code path would prevent this from breaking again. If the regression is really significant, then I can put in a few if-elses

[kageiit]

This is about covering all the cases consumers can write to disk for jar'ing. We should ideally avoid work unless its needed. Unzipping every single classes jar to obtain code coverage is a significant performance regression especially if the classes directory is already available. Consumers cannot spool to jar if postprocess_classes_commands are active as well, so this sort of change will break build time slas for them

[tdrhq]

To be clear, it shouldn't be every single jar file, it should only be the ones that have the given test in their tests = [] argument. (So, ideally just 2-3 at most even in a big repository)

[kageiit]

I dont understand what you are saying. The classes directory of a rule was replaced with rule.getPathToOutput(); which is getting unzipped for every rule.

[kageiit]

we have several thousand rules that have associated tests with them. We should not be required to unzip the jars for computing code coverage when the unzipped classes directory is already available. We also dont spool directly to jar

[tdrhq]

@kageiit I see, so your concern is with CI runs where you run all the tests, correct? Local runs probably don't get affected as much because people probably run a few targets at a time. Just understanding the situation right now.

[kageiit]

Yes. That is correct

[kageiit]

@tdrhq any updates on this?

[tdrhq]

will be doing this soon, I would still love some kind of number to get a sense of what percentage increase we're talking about, an approximation would do

[kageiit]

Approximately 15 ~ 30 seconds more when running coverage on several thousands of test rules. Overall test run times are around 8 ~ 10 min. This is because code coverage is purely single threaded and happens at the very end after all test rules are run. The unzip has to happen for every build target that underwent tests. The number of test rules are only increasing and would be great to avoid this extra unzip time since the classes are already available

[kageiit]

@tdrhq any updates?

[tdrhq]

woops sorry, I'll work on this today

[tdrhq]

Fixed in a4cd1b0

Let me know if it works

[kageiit]

Going to pull this in by tomorrow. Will report back

[kageiit]

@tdrhq We are seeing an exception with the latest buck version

TESTS PASSED
Failed on step emma_report with an exception:
buck-out/bin/freight/freight-authentication/lib__src_release__classes does not exist
java.lang.IllegalStateException: buck-out/bin/freight/freight-authentication/lib__src_release__classes does not exist
	at com.google.common.base.Preconditions.checkState(Preconditions.java:444)
	at com.facebook.buck.jvm.java.GenerateCodeCoverageReportStep.populateClassesDir(GenerateCodeCoverageReportStep.java:189)
	at com.facebook.buck.jvm.java.GenerateCodeCoverageReportStep.execute(GenerateCodeCoverageReportStep.java:98)
	at com.facebook.buck.step.DefaultStepRunner.runStepForBuildTarget(DefaultStepRunner.java:47)
	at com.facebook.buck.cli.TestRunning.runTests(TestRunning.java:437)
	at com.facebook.buck.cli.TestCommand.runTestsInternal(TestCommand.java:316)
	at com.facebook.buck.cli.TestCommand.runWithoutHelp(TestCommand.java:580)
	at com.facebook.buck.cli.AbstractCommand.run(AbstractCommand.java:205)
	at com.facebook.buck.cli.BuckCommand.run(BuckCommand.java:91)
	at com.facebook.buck.cli.Main.runMainWithExitCode(Main.java:1287)
	at com.facebook.buck.cli.Main.runMainThenExit(Main.java:706)
	at com.facebook.buck.cli.Main.main(Main.java:1884)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.facebook.buck.cli.bootstrapper.ClassLoaderBootstrapper.main(ClassLoaderBootstrapper.java:62)

[felipecsl]

We're seeing the exception above too. Are there any workarounds for it?

[felipecsl]

This seems to be fixed with jar_spool_mode = direct_to_jar in the buckconfig file