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

Create the default project only when it is necessary. #3140

Merged
merged 1 commit into from May 2, 2024

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Apr 26, 2024

  • Default project is configured with a (default) JRE, that will produce
    unwanted symbols when other projects don't use the default JRE

  • Place default project creation logic into the invisible project
    importer since that is the importer requiring it

  • Continue to support some special cases where default project should be
    created (eg. Maven/Gradle project & opening a standalone file)

  • Update testcases & add a testcase

  • Fixes Remove duplicate listing of Symbols redhat-developer/vscode-java#3452

@snjeza , let me know if the PR seems reasonable, or would break something.

The basic issue is when you have some Maven/Gradle project that sets a certain Java version, that is different from the default version (eg. java.home or from java.configuration.runtimes with default set to true), then you'll get duplicate symbols.

At first I wanted to avoid creating the default project when a Maven/Gradle project is used, but apparently we actually have support for opening external files through the default project in those cases.

My next solution was to just take into account the Java versions used by the projects and set that.

Update: Now I've polished up the original solution so the default project is created only when needed.

@rgrunber rgrunber added this to the End May 2024 milestone Apr 26, 2024
@rgrunber rgrunber requested a review from snjeza April 26, 2024 20:39
snjeza
snjeza previously approved these changes Apr 26, 2024
Copy link
Contributor

@snjeza snjeza left a comment

Choose a reason for hiding this comment

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

LGTM

@testforstephen
Copy link
Contributor

If users explicitly set a default jdk in the setting "java.configuration.runtimes", this will make it fail. How about setting the default JDK to the lowest configured for managed projects only when no default jdk is specified by user?

@rgrunber
Copy link
Contributor Author

rgrunber commented Apr 29, 2024

@fbricon isn't a fan of this approach. He mentioned wanting to just load the default project on-demand. I have a PR that attempted to not load the default project at all (when a managed one is present), but I just need to detect if a "standalone" file is being added to a managed project, and issue the project creation for it. If I can get that working, I can probably just update this PR with the new logic.

@rgrunber rgrunber dismissed snjeza’s stale review April 29, 2024 15:02

Need to reimplement this with the older approach.

@rgrunber rgrunber changed the title Set default JDK to the lowest configured for managed projects. Create the default project only when it is necessary. Apr 30, 2024
@rgrunber
Copy link
Contributor Author

rgrunber commented May 1, 2024

@snjeza let me know if this new change looks fine. The only thing I want to try and see is if there's any performance improvement from avoiding creation of the default project in most cases.

Copy link
Contributor

@snjeza snjeza left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@rgrunber
Copy link
Contributor Author

rgrunber commented May 2, 2024

So I didn't even notice before, but our standard JDT-LS pr verification (which runs all tests) takes about 35min altogether when successful. This PR took 25min. Comparing against just something like MavenProjectImporterTest shows the tests go from taking about 33s to 20s. Basically, this PR definitely improves things at build-time but I'm more interested in what the gain will be at runtime.

Testing against spring-petclinic (Maven)

Before

Metric Run 1 Run 2 Run 3 Run 4 Run 5
time.projectsinitialized (ms) 6363 6389 6162 6494 5925
time.serviceready (ms) 6776 6719 6508 6816 6615
time.buildFinished (ms) 25864 25055 25436 26136 22647

Medians : 6363, 6719, 25436

After

Metric Run 1 Run 2 Run 3 Run 4 Run 5
time.projectsinitialized (ms) 5468 5382 5543 5512 5061
time.serviceready (ms) 6199 5738 6100 6227 5543
time.buildFinished (ms) 22779 21927 21893 22035 24580

Medians: 5468, 6100, 22035

Given that I don't think creating the default project scales with the size of the project being imported, I think we're saving a constant 1s of overall project import time. I don't really know what to make of the build time. The build times also appear to imply some savings.

- Default project is configured with a (default) JRE, that will produce
  unwanted symbols when other projects don't use the default JRE
- Place default project creation logic into the invisible project
  importer since that is the importer requiring it
- Continue to support some special cases where default project should be
  created (eg. Maven/Gradle project & opening a standalone file)
- Update testcases & add a testcase

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@snjeza
Copy link
Contributor

snjeza commented May 2, 2024

I don't really know what to make of the build time. The build times also appear to imply some savings.

Most tests create and delete this project for each test method. There are 1800+ tests.
Nice catch :)

@rgrunber rgrunber merged commit 64dca83 into eclipse-jdtls:master May 2, 2024
6 of 7 checks passed
@rgrunber rgrunber deleted the fix-3452 branch May 2, 2024 20:50
@fbricon
Copy link
Contributor

fbricon commented May 2, 2024

yes-baby

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.

Remove duplicate listing of Symbols
4 participants