Feuermagier / autograder

Automatic grading of student's Java code
MIT License
13 stars 7 forks source link

Rewrite uses search for performance, simplicity & correctness #509

Closed Feuermagier closed 2 months ago

Feuermagier commented 2 months ago

I did some profiling for the A1 test and found that the currently slowest check by far is the UnusedCodeElementCheck (and the UseDifferentVisiblityCheck, which also searches for code element uses). On my machine (i7-12700K) it looks like this:

image

With a profiler attached, the entire analysis takes about 17.5s, of which the UnusedCodeElementCheck takes ~7s, and the UseDifferentVisiblityCheck takes another 2.5s. For reference, the next slowest checks are the ConstantNamingAndQualifierCheck (0.84s), ReassignedParameterCheck (0.78s), and the UnusedImportCheck (0.58s). Model building takes only about 1s, which is faster than the compilation step! (probably due to laziness within Spoon)

Anyway, we should improve the performance of the usage search. The current implementation sweeps across the entire model for each code element that we want usage information for, which leads to a runtime of something like O(n²). The goal of this PR is to rewrite usage search with a single pass over the model and a caching strategy for different checks.

Feuermagier commented 2 months ago

Also, there is a failure case within the current usage search around instanceof patterns. This is valid Java code, in which i is used:

Object o = "Hello";
if (!(o instanceof String i)) {
    throw new IllegalArgumentException();
}
System.out.println(i);

The pattern variable i is introduced to the outer scope since the then-block cannot complete normally, so we statically know that o is a String when printing. (see JLS §6.3.2 https://docs.oracle.com/javase/specs/jls/se22/html/jls-6.html#jls-6.3.2)

Feuermagier commented 2 months ago

@Luro02 I've just seen that you are doing something similar in #503. Am I right in that this rewrite is only for readability/code structure, and not performance? If so, I think we should prefer my implementation since it is significantly faster than the old implementation in first tests.

Luro02 commented 2 months ago

Mine is just a new api, so we don't have like 20 different methods in SpoonUtil.

Feuermagier commented 2 months ago

The implementation now works in all cases (i.e. passes all existing tests and some more, and generates the same problems on the A1 task as the main branch, aside from a bug on main). It's also a lot faster compared to main and the scope-limiting implemented in #503:

Caching Uses:

Executed in 4.30 secs fish external usr time 28.60 secs 411.00 micros 28.60 secs sys time 1.70 secs 216.00 micros 1.70 secs

Scope-Limiting:

Executed in 12.01 secs fish external usr time 36.66 secs 643.00 micros 36.66 secs sys time 1.75 secs 164.00 micros 1.75 secs

Baseline on main:

Executed in 14.50 secs fish external usr time 40.10 secs 692.00 micros 40.10 secs sys time 1.62 secs 0.00 micros 1.62 secs

Looking at the profiler, the checks take now less than 25% (1.2s) of the total runtime (5.3s), with the remaining large chunks being Spoon's model building (0.9s), PMD (1s), javac (0.9s), and Lingua (0.3s), which is disabled in practice anyway: image

The API is in no way finished, and I'm very unhappy with the current split of the model-related functionality into SpoonUtil, CodeModel, and now Uses/MethodHierarchy. It's especially inconvenient that the CodeModel has to be passed around everywhere where Uses may be needed. I've implemented a shortcut by caching the CodeModel inside the root CtPackage's metadata, but that feels hacky and may break sometime down the line.