ArC - parallelize resource generation

[mkouba]

I have to admit that I have no "scientifict measurements". My observations while working on this PR are that for small apps the performance is more or less identical and for large apps (such as 003-quarkus-many-extensions) the difference was ~15% on average.

[manovotn]

I'll take a look tomorrow if that's ok

[mkouba]

I'll take a look tomorrow if that's ok

No problem at all. It's not a high priority...

[Ladicek]

The changes are pretty straightforward, but the code duplication on a few places is unfortunate.

Also, not sure why you need to precompute those names (prefill*) before generating the resources, but I'm sure there's a good reason.

What I find most interesting is that you can actually achieve speedup with this. That suggests that the concurrent build step execution framework can't utilize the CPU fully, probably because a lot of build steps do blocking operations?

[Sanne]

Nice idea :)
But could you avoid adding a configuration property? I'd say just enable it...

[mkouba]

Also, not sure why you need to precompute those names (prefill*) before generating the resources, but I'm sure there's a good reason.

We need to generate the names first because they're used in the ComponentsProviderGenerator. If we don't pregenerate the names we would have to wait for all bean and observer generators to finish and then start the generation of the ComponentsProvider (i.e. no parallel execution). Note that the generated ComponentsProvider is one of the largest class ArC generates and so it should go first. I'll add some javadoc to the method.

What I find most interesting is that you can actually achieve speedup with this. That suggests that the concurrent build step execution framework can't utilize the CPU fully, probably because a lot of build steps do blocking operations?

There are build steps whose dependencies/dependents prevent or limit the concurrent execution. Typical examples are ClassTransformingBuildStep#handleClassTransformation and JarResultBuildStep#buildRunnerJar. For these build steps it might help to try to parallelize the execution inside the build step. And FYI we already do this in the ClassTransformingBuildStep#handleClassTransformation.

[mkouba]

But could you avoid adding a configuration property? I'd say just enable it...

It's just a safeguard so that it can be disabled in case of an oversight that would cause some concurrency issues. And it's enabled by default.

[manovotn]

I don't have any objections to this.
The idea is solid, doesn't hurt for smaller deployments and if it helps in beefy scenarios, it's always a plus. So +1

EDIT: I'd also be fine with not using the config option and just making it default, but I understand why you wanted to keep that in. Perhaps we could have it straight up deprecated so that we can eventually get rid of it once we see this doesn't cause troubles?
The less config options, the better :-)

[Sanne]

But could you avoid adding a configuration property? I'd say just enable it...

It's just a safeguard so that it can be disabled in case of an oversight that would cause some concurrency issues. And it's enabled by default.

yes I understand that, but I often think we should be a bit more confident with our changes and then stand behind them - or when not test them more.
Having additional properties won't save the day for most people as they won't figure out a relation from the random problem to needing to set this.

[manovotn]

Having additional properties won't save the day for most people as they won't figure out a relation from the random problem to needing to set this.

I think the idea here is that if the change is to cause some problems, users will report it and we can then direct them to use this config option as a temporary workaround before we have a proper fix. So it effectively unblocks the user.
And I understand the decision, I would just like to see a way to gradually lower the amount of configs as we become more confident about the changes we made.

[Sanne]

yea I agree in principle - feel free to ignore me, but at least deprecating the new property would be nice.
FWIW I've used system properties for similar things, perhaps that's more suitable and doesn't distract in documentation.

[mkouba]

@manovotn @Sanne all your comments about the config item make a lot of sense. I'll switch to a system property. I want to keep it so that we can also easily test the difference.

[mkouba]

^ Are these known flaky tests? @gsmet

[quarkus-bot]

Failing Jobs - Building f85208e

Status	Name	Step	Failures	Logs	Raw logs
✖	Devtools Tests - JDK 11	Build	Failures	Logs	Raw logs
✖	Devtools Tests - JDK 11 Windows	Build	Failures	Logs	Raw logs
✖	Devtools Tests - JDK 17	Build	Failures	Logs	Raw logs
✔️	Gradle Tests - JDK 11
✖	Gradle Tests - JDK 11 Windows	Build	Failures	Logs	Raw logs
✖	Native Tests - Windows - RESTEasy Jackson	Run actions/checkout@v2	⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Devtools Tests - JDK 11 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

✖ io.quarkus.platform.catalog.ExtensionProcessorTest.testKotlinMetadata line 62 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting map:

---

⚙️ Devtools Tests - JDK 11 Windows #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

✖ io.quarkus.platform.catalog.ExtensionProcessorTest.testKotlinMetadata line 62 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting map:

---

⚙️ Devtools Tests - JDK 17 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

✖ io.quarkus.platform.catalog.ExtensionProcessorTest.testKotlinMetadata line 62 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting map:

---

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

[gsmet]

The failing tests above are either flaky or have been already fixed in main and are not related to this. So feel free to merge when ready.