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

Rewrite uses search for performance, simplicity & correctness #509

Merged
merged 14 commits into from
May 13, 2024

Conversation

Feuermagier
Copy link
Owner

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
Copy link
Owner Author

Feuermagier commented Apr 25, 2024

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
Copy link
Owner Author

@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
Copy link
Collaborator

Luro02 commented Apr 26, 2024

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

Copy link
Collaborator

@Luro02 Luro02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my PC the UnusedCodeElement check is the slowest one as well:
image

(not sure about the reassigned parameter check, might have introduced some slow down in my fork)

@Feuermagier Feuermagier self-assigned this Apr 29, 2024
@Feuermagier
Copy link
Owner Author

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.

@Feuermagier Feuermagier requested a review from Luro02 May 4, 2024 20:33
@Feuermagier Feuermagier marked this pull request as ready for review May 10, 2024 00:18
@Feuermagier Feuermagier merged commit 5ba130c into main May 13, 2024
4 checks passed
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.

None yet

2 participants