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

Optimize config generation #32209

Closed
aloubyansky opened this issue Mar 29, 2023 · 24 comments · Fixed by #32276 or #33228
Closed

Optimize config generation #32209

aloubyansky opened this issue Mar 29, 2023 · 24 comments · Fixed by #32276 or #33228
Assignees
Labels
area/config kind/enhancement New feature or request

Comments

@aloubyansky
Copy link
Member

aloubyansky commented Mar 29, 2023

Description

After merging #30506 io.quarkus.maven.it.DevMojoIT.testThatTheApplicationIsReloadedMultiModule started to fail with the OOME. Although these dev mode tests are run with -Xmx128m, they do highlight an issue in the way our config classes are generated currently.

Implementation ideas

As an alternative to the current impl, @radcortez suggested to switch to the ConfigMapping approach instead in https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/OOME.20in.20main/near/343476381

@aloubyansky aloubyansky added kind/enhancement New feature or request area/config labels Mar 29, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 29, 2023

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip

This message is automatically generated by a bot.

@gsmet gsmet self-assigned this Mar 29, 2023
@gsmet
Copy link
Member

gsmet commented Mar 29, 2023

I will experiment with this soon and report back.

@geoand
Copy link
Contributor

geoand commented Mar 29, 2023

#32209 changes RESTEasy Reactive configuration to @ConfigMapping

@aloubyansky
Copy link
Member Author

@geoand I think you meant #32224

@geoand
Copy link
Contributor

geoand commented Mar 29, 2023

Oops :)

@aloubyansky
Copy link
Member Author

I don't think this is done yet on "full scale". I'll re-open it for further evaluation.

@aloubyansky aloubyansky reopened this Apr 11, 2023
@yrodiere
Copy link
Member

Thanks for catching this, yes GitHub auto-closed this when I merged the PR but that's wrong.

@radcortez
Copy link
Member

It would be interesting to measure if the changes are showing any impact. I'm pretty sure they will, but nothing like confirming it :)

@aloubyansky
Copy link
Member Author

One way of testing an impact would be re-running the test mentioned in the description with -Xmx128m.

@gsmet gsmet removed this from the 3.0.0.Final milestone Apr 12, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Aug 2, 2023
@radcortez radcortez reopened this Aug 2, 2023
@radcortez radcortez self-assigned this Aug 2, 2023
@gsmet
Copy link
Member

gsmet commented Aug 2, 2023

FWIW, I just tried to lower the memory for these tests again and things look worse than before as I can't even get to the point of starting Quarkus: the bootstrap itself requires more than 128 MB of heap space.

Failure is in:

Caused by: java.lang.OutOfMemoryError: Java heap space
	at org.apache.maven.model.Dependency.clone(Dependency.java:246)
	at org.apache.maven.model.DependencyManagement.clone(DependencyManagement.java:75)
	at org.apache.maven.model.building.ModelCacheTag$2.intoCache(ModelCacheTag.java:109)
	at org.apache.maven.model.building.ModelCacheTag$2.fromCache(ModelCacheTag.java:114)
	at org.apache.maven.model.building.ModelCacheTag$2.fromCache(ModelCacheTag.java:95)
	at org.apache.maven.model.building.DefaultModelBuilder.getCache(DefaultModelBuilder.java:1252)
	at org.apache.maven.model.building.DefaultModelBuilder.importDependencyManagement(DefaultModelBuilder.java:1151)
	at org.apache.maven.model.building.DefaultModelBuilder.build(DefaultModelBuilder.java:486)
	at org.apache.maven.model.building.DefaultModelBuilder.build(DefaultModelBuilder.java:410)
	at org.apache.maven.model.building.DefaultModelBuilder.build(DefaultModelBuilder.java:243)
	at io.quarkus.bootstrap.resolver.maven.MavenModelBuilder.build(MavenModelBuilder.java:63)
	at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:281)
	at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.readArtifactDescriptor(DefaultArtifactDescriptorReader.java:172)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.resolveCachedArtifactDescriptor(DfDependencyCollector.java:382)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.getArtifactDescriptorResult(DfDependencyCollector.java:368)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.processDependency(DfDependencyCollector.java:218)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.processDependency(DfDependencyCollector.java:156)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.process(DfDependencyCollector.java:138)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.doRecurse(DfDependencyCollector.java:343)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.processDependency(DfDependencyCollector.java:277)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.processDependency(DfDependencyCollector.java:156)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.process(DfDependencyCollector.java:138)
	at org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector.doCollectDependencies(DfDependencyCollector.java:108)
	at org.eclipse.aether.internal.impl.collect.DependencyCollectorDelegate.collectDependencies(DependencyCollectorDelegate.java:222)
	at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.collectDependencies(DefaultDependencyCollector.java:87)
	at org.eclipse.aether.internal.impl.DefaultRepositorySystem.collectDependencies(DefaultRepositorySystem.java:305)
	at io.quarkus.bootstrap.resolver.maven.MavenArtifactResolver.collectManagedDependencies(MavenArtifactResolver.java:307)
	at io.quarkus.bootstrap.resolver.maven.ApplicationDependencyTreeResolver.collectDependencies(ApplicationDependencyTreeResolver.java:592)
	at io.quarkus.bootstrap.resolver.maven.ApplicationDependencyTreeResolver.injectDeploymentDependencies(ApplicationDependencyTreeResolver.java:493)
	at io.quarkus.bootstrap.resolver.maven.ApplicationDependencyTreeResolver.resolve(ApplicationDependencyTreeResolver.java:197)
	at io.quarkus.bootstrap.resolver.BootstrapAppModelResolver.buildAppModel(BootstrapAppModelResolver.java:321)
	at io.quarkus.bootstrap.resolver.BootstrapAppModelResolver.doResolveModel(BootstrapAppModelResolver.java:286)

@gsmet
Copy link
Member

gsmet commented Aug 2, 2023

/cc @aloubyansky ^

@gsmet
Copy link
Member

gsmet commented Aug 2, 2023

Above stacktrace is testing:

[ERROR] TestMojoIT>LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule:34 » ConditionTimeout Condition with lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.

@aloubyansky
Copy link
Member Author

When it comes to TestMojoIT, there is only one test that loads only 4 workspace modules.

@gsmet
Copy link
Member

gsmet commented Aug 4, 2023

From what I can see I have failure in the Failsafe tests also if I disable Surefire.

@aloubyansky
Copy link
Member Author

@gsmet
Copy link
Member

gsmet commented Aug 4, 2023

Yes. I reduced it to 64M first to see how it was doing, then raised it to 128M, which was our initial value, before we started having issues IIRC.

@aloubyansky
Copy link
Member Author

I ran io.quarkus.maven.it.DevMojoIT.testThatTheApplicationIsReloadedMultiModule with 128M w/o failing.

@aloubyansky
Copy link
Member Author

Ah, hold on, I'll re-test.

@aloubyansky
Copy link
Member Author

Yes, still OOME.

@aloubyansky
Copy link
Member Author

It doesn't look like it's the size of the workspace though, it loads only for modules. It must be something else.

@gsmet
Copy link
Member

gsmet commented Aug 4, 2023

I haven't tried to dump the memory on OOME. Do you want me to do that?

@aloubyansky
Copy link
Member Author

Sure, thanks.

@gsmet
Copy link
Member

gsmet commented Aug 9, 2023

@radcortez
Copy link
Member

We did many optimizations in the last few releases with:

And many more... I think we can close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/enhancement New feature or request
Projects
None yet
5 participants