Skip completely inaccessible classpath elements in package caching#4910
Open
HannesWell wants to merge 1 commit into
Open
Skip completely inaccessible classpath elements in package caching#4910HannesWell wants to merge 1 commit into
HannesWell wants to merge 1 commit into
Conversation
HannesWell
force-pushed
the
skip-inaccessible-entries
branch
from
cb39639 to
29b8222
Compare
HannesWell
force-pushed
the
skip-inaccessible-entries
branch
from
29b8222 to
8e20be9
Compare
HannesWell
force-pushed
the
skip-inaccessible-entries
branch
3 times, most recently
from
3631389 to
bd6951a
Compare
Contributor
|
@HannesWell thank you for the PR!. As discussed in the last Community Call, I tested this in our big workspace and it brings the compilation time (full build) from 1226s down to 1187s (~3% improvement).
Here are the VisualVM Samples |
Contributor
Author
Thanks for the measurements! @iloveeclipse should this proceed? |
HannesWell
force-pushed
the
skip-inaccessible-entries
branch
from
bd6951a to
d8aa1ed
Compare
Member
|
I simply have no time. Maybe @srikanth-sankaran could have a look? |
There was a problem hiding this comment.
Pull request overview
This PR optimizes JavaProjectElementInfo package-cache creation by skipping package name initialization for classpath entries that are fully forbidden via access rules (a single **/* non-accessible rule). The intent is to reduce time spent in a hot loop when many completely inaccessible entries exist (notably in Plug-in projects).
Changes:
- Skip
initializePackageNames(...)when a classpath entry is detected as “completely non-accessible” (ForbiddenReferencewith a single**/*rule). - Update model tests to reflect the new behavior (fully forbidden entries no longer yield non-accessible lookup answers / access-restriction diagnostics).
- Adjust expected compiler problem output in access restriction tests accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaProjectElementInfo.java |
Adds detection of fully forbidden classpath entries and skips package name caching for those roots. |
org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/NameLookupTests2.java |
Changes expectation from “found but non-accessible” to “not found” for a fully forbidden library. |
org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/AccessRestrictionsTests.java |
Changes expected problems from access restriction diagnostics to unresolved-type errors for a fully forbidden library. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+247
to
+250
| if (!(reverseMap.get(root) instanceof ClasspathEntry cpEntry) | ||
| || !isCompletelyNonAccessible(cpEntry)) { | ||
| initializePackageNames(root, fragmentsCache); | ||
| } |
Comment on lines
+265
to
+274
| private static final char[] ALL_ELEMENTS = {'*', '*', '/', '*'}; | ||
|
|
||
| private boolean isCompletelyNonAccessible(ClasspathEntry cpEntry) { | ||
| AccessRuleSet accessRules = cpEntry.getAccessRuleSet(); | ||
| if (accessRules != null) { | ||
| AccessRule[] rules = accessRules.getAccessRules(); | ||
| if (rules.length == 1) { | ||
| AccessRule rule = rules[0]; | ||
| if (rule.getProblemId() == IProblem.ForbiddenReference && Arrays.equals(ALL_ELEMENTS, rule.pattern)) { | ||
| return true; |
| null); | ||
| assertNotNull("Expected to find type", answer); | ||
| assertTrue("Expected type to be non-accessible", answer.isNonAccessible()); | ||
| assertNull("Expected to not find type", answer); |
Comment on lines
1076
to
+1117
| public void test011() throws CoreException { | ||
| ICompilationUnit y = null; | ||
| try { | ||
| createJavaProject( | ||
| "P1", | ||
| new String[] {"src"}, | ||
| new String[] {"JCL18_LIB", "/AccessRestrictions/lib.jar"}, | ||
| new String[][]{{}, {}}, | ||
| new String[][]{{}, {"**/*"}}, | ||
| null/*no project*/, | ||
| null/*no inclusion pattern*/, | ||
| null/*no exclusion pattern*/, | ||
| null/*no exported project*/, | ||
| "bin", | ||
| null/*no source outputs*/, | ||
| null/*no inclusion pattern*/, | ||
| null/*no exclusion pattern*/, | ||
| CompilerOptions.getFirstSupportedJavaVersion()); | ||
| this.problemRequestor = new ProblemRequestor(); | ||
| y = getWorkingCopy( | ||
| "/P1/src/q/Y.java", | ||
| "package q;\n" + | ||
| "public class Y {\n" + | ||
| " void foo() {\n" + | ||
| " p.X x = new p.X();\n" + | ||
| " x.foo();\n" + | ||
| " if (x.m > 0) {}\n" + | ||
| " }\n" + | ||
| "}" | ||
| ); | ||
| assertProblems( | ||
| "Unexpected problems", | ||
| "----------\n" + | ||
| "1. ERROR in /P1/src/q/Y.java (at line 4)\n" + | ||
| " p.X x = new p.X();\n" + | ||
| " ^^^\n" + | ||
| "Access restriction: The type \'X\' is not accessible (restriction on required library \'AccessRestrictions/lib.jar\')\n" + | ||
| " ^\n" + | ||
| "p cannot be resolved to a type\n" + | ||
| "----------\n" + | ||
| "2. ERROR in /P1/src/q/Y.java (at line 4)\n" + | ||
| " p.X x = new p.X();\n" + | ||
| " ^^^\n" + | ||
| "Access restriction: The type \'X\' is not accessible (restriction on required library \'AccessRestrictions/lib.jar\')\n" + | ||
| "----------\n" + | ||
| "3. ERROR in /P1/src/q/Y.java (at line 4)\n" + | ||
| " p.X x = new p.X();\n" + | ||
| " ^^^\n" + | ||
| "Access restriction: The constructor \'X()\' is not accessible (restriction on required library \'AccessRestrictions/lib.jar\')\n" + | ||
| "----------\n" + | ||
| "4. ERROR in /P1/src/q/Y.java (at line 5)\n" + | ||
| " x.foo();\n" + | ||
| " ^^^\n" + | ||
| "Access restriction: The method \'X.foo()\' is not accessible (restriction on required library \'AccessRestrictions/lib.jar\')\n" + | ||
| "----------\n" + | ||
| "5. ERROR in /P1/src/q/Y.java (at line 6)\n" + | ||
| " if (x.m > 0) {}\n" + | ||
| " ^\n" + | ||
| "Access restriction: The field \'X.m\' is not accessible (restriction on required library \'AccessRestrictions/lib.jar\')\n" + | ||
| " ^\n" + | ||
| "p cannot be resolved to a type\n" + |
Contributor
@jarthana would be the best person to review. Jay can you take a look ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
Skip classpath entries that are completely inaccessible in package cache creation.
This could help to counter the increased number of inaccessible inaccessible entries for Plug-in projects, as discussed in
This should shorten a hot loop I identified in a first debugging/sampling session.
This is currently a draft as it needs further testing and maybe further adjustments.
Author checklist