improve: generate crds in parallel and optimize code #4644

nicoloboschi · 2022-12-05T17:32:07Z

Description

In my project I have 10 CRDs classes.
I've taken a CPU flamegraph and I figured out the usage of java streams api adds a relevant overhead while visiting specs/status classes via reflection.

Also the visitors can access the crd in parallel since the base typeref is stateless.

Changes included:

Enable concurrent crd visitors using nCPU threads.
Moved some hot path code from using streams to old plain java for-loop

FYI I did a similar fix in sundrio: sundrio/sundrio#352

The net result of this patch is that generation time moved from ~30 seconds to to ~11 seconds.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

andreaTP · 2022-12-05T18:16:58Z

Hi @nicoloboschi , thanks for this PR! This sounds super interesting indeed!
But, since I have already experienced issues with concurrency + sundrio + crd-gen, I need to summon @metacosm and @iocanel to review this change 🙏

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java

andreaTP · 2022-12-06T15:56:04Z

@nicoloboschi are you still actively working on this? if that's the case, would you mind converting to Draft?

nicoloboschi · 2022-12-06T16:37:58Z

@nicoloboschi are you still actively working on this? if that's the case, would you mind converting to Draft?

it's ready for review

iocanel · 2022-12-12T11:21:52Z

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java

+      List<CompletableFuture<Void>> futures = new ArrayList<>();
+      if (config.specClassName().isPresent()) {
+        futures.add(CompletableFuture.runAsync(() -> {
+          TypeDefBuilder builder = new TypeDefBuilder(def);


@andreaTP mentioned a change in semantics, which is true.
However, it shouldn't make a difference as in this particular case we are not interested in building something (we completely ignore that), but instead we just need to traverse the object graph.

So, in my book it's acceptable but I would add a comment so that it's clear to people less familiar with the code.

Since, we are interested in optimizing things, I would also try to pass both visitors in a single accept invocation (it does accept var-arg). Since, that would reduce the graph traversals to half it might make a difference for large objects.

However, it shouldn't make a difference as in this particular case we are not interested in building something (we completely ignore that), but instead we just need to traverse the object graph.

yes, exactly, we just do a LOT of recursive calls and the only output is stored in the visitor object itself (getPath())

@iocanel I've already tried with the var-args solution but it doesn't help.

Sharing my dummy benchmark numbers

using quarkus-operator-sdk mojo to generate crd

jdk 17

Intel mac 6 core

8 CRD in the project with ~200 props in total, spec and status classes in each one

3 rounds per solution

Results:

current version: ~40s

concurrency of 5: ~18s

var args: ~45s

iocanel

Overall, looks good.
Added a comment with two suggestion, which I consider nice ot have.

sonarcloud · 2022-12-14T15:37:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

83.6% Coverage
0.0% Duplication

nicoloboschi · 2022-12-22T09:39:36Z

@iocanel any chance we can merge this?

this is my latest comment about performances comparison

using quarkus-operator-sdk mojo to generate crd
jdk 17
Intel mac 6 core
8 CRD in the project with ~200 props in total, spec and status classes in each one
3 rounds per solution

Results:

current version: ~40s
concurrency of 5: ~18s
var args: ~45s

nicoloboschi · 2023-01-14T00:04:29Z

@iocanel @andreaTP any chance to see this pull moving forward? 🤞

andreaTP · 2023-01-16T16:39:31Z

Hi @nicoloboschi and thanks again for your effort!
I'm leaving space to @metacosm and @iocanel here, maybe also @manusa wants to chip into this.

manusa · 2023-01-17T12:51:46Z

Hi @nicoloboschi
I still need to review this and analyze if it might cause trouble in certain environments. AFAIK running the generation in several parallel processes might lead to inconsistent results.
One of the options is to make this a configuration option.

nicoloboschi · 2023-01-17T17:04:56Z

Hi @nicoloboschi I still need to review this and analyze if it might cause trouble in certain environments. AFAIK running the generation in several parallel processes might lead to inconsistent results. One of the options is to make this a configuration option.

@manusa I can totally add a flag. I haven't find any configuration present at the moment. Do you have any suggestion on how to make it configurable?

For example, I could add a system property but I'm not sure how it is supposed to be set if someone is using the crd-generator-apt module

andreaTP · 2023-01-17T17:28:20Z

Do you have any suggestion on how to make it configurable?

Currently, the crd generator is purely using annotations and has no additional configuration:
https://github.com/fabric8io/kubernetes-client/tree/009974fe5fe634cbd6d1210780b99ff82e251f2d/crd-generator/api/src/main/java/io/fabric8/crd/generator/annotation

nicoloboschi · 2023-01-17T19:14:04Z

I've made it configurable and disabled by default.
I added an option in CRDGenerator for enabling the parallel execution.
For the apt processor I've added an option io.fabric8.crd.generator.parallel.
I added the apt processor option example in the test pom.xml

<compilerArgs>
            <arg>-Aio.fabric8.crd.generator.parallel=false</arg>
</compilerArgs>

I've added CRDGenerator tests with parallel enabled
I tested locally the crd-generator/tests suite with parallel enabled but I've left it disabled since it's better to test the default configuration in this case

@andreaTP let me know if I missed something

andreaTP · 2023-01-17T20:54:16Z

I just skimmed through on the phone, and, making parallel execution optional makes a lot of sense.
I think that the Maven setting is a bit "hidden" and you probably want to mention it in the docs.

I might be wrong, but it looks like there are a few tiny bits that are not source compatible (which is appropriate to merge before 6.4) and @metacosm would need to have a look and follow up in the Quarkus Operator SDK.

nicoloboschi · 2023-01-18T09:44:03Z

I created the related pr in for the quarkus sdk: quarkiverse/quarkus-operator-sdk#488 (it's only a draft since we need this to get released)

crd-generator/test/pom.xml

andreaTP · 2023-01-18T09:52:15Z

Thanks for the follow-ups @nicoloboschi , LGTM.

metacosm

LGTM but I haven't checked downstream projects yet.

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java

nicoloboschi · 2023-01-26T18:06:38Z

@gastaldi I've addressed your comments, please review it again

gastaldi

LGTM. I'd appreciate if you could please squash the commits. Good job!

gastaldi · 2023-01-26T18:25:50Z

doc/CRD-generator.md

+## Experimental
+
+### Generate CRDs in parallel
+Starting from 6.4, it's possible to speed up the CRDs generation by using parallel computation.


6.4.0 is already released, since this change doesn't break the API it could be introduced in a patch 6.4.x release

gastaldi · 2023-01-26T18:28:47Z

crd-generator/api/src/main/java/io/fabric8/crd/generator/CRDGenerator.java

@@ -68,6 +69,11 @@ public CRDGenerator withOutput(CRDOutput<? extends OutputStream> output) {
    return this;
  }

+  public CRDGenerator withParallelGenerationEnabled(boolean parallel) {


Just an idea: what if the ExecutorService is passed instead? That would give a better control over the execution.

In general I think this makes sense.
In this specific case, given the crd generation is a short-running task, I don't believe it's worth to manage the thread pool at higher level

nicoloboschi · 2023-01-27T08:22:07Z

Fixed the doc, rebased to the current master and squashed @gastaldi

manusa

Awesome work, thx!

sonarcloud · 2023-01-27T10:08:21Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

64.5% Coverage
2.4% Duplication

nicoloboschi marked this pull request as ready for review December 5, 2022 17:33

andreaTP requested review from metacosm and iocanel December 5, 2022 18:15

nicoloboschi force-pushed the crd-parallel branch from e18cfa1 to 1b39934 Compare December 6, 2022 14:05

andreaTP reviewed Dec 6, 2022

View reviewed changes

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java Outdated Show resolved Hide resolved

iocanel reviewed Dec 12, 2022

View reviewed changes

iocanel approved these changes Dec 12, 2022

View reviewed changes

manusa requested review from manusa, oscerd, rohanKanojia, shawkins and sunix as code owners January 11, 2023 11:16

nicoloboschi mentioned this pull request Jan 18, 2023

Option to enable parallel CRD generation quarkiverse/quarkus-operator-sdk#488

Merged

nicoloboschi force-pushed the crd-parallel branch 2 times, most recently from 99e5191 to 8d8244e Compare January 18, 2023 09:42

andreaTP reviewed Jan 18, 2023

View reviewed changes

crd-generator/test/pom.xml Show resolved Hide resolved

andreaTP approved these changes Jan 18, 2023

View reviewed changes

metacosm approved these changes Jan 18, 2023

View reviewed changes

gastaldi reviewed Jan 26, 2023

View reviewed changes

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java Outdated Show resolved Hide resolved

gastaldi reviewed Jan 26, 2023

View reviewed changes

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java Outdated Show resolved Hide resolved

gastaldi approved these changes Jan 26, 2023

View reviewed changes

improve: generate crds in parallel and optimize code

cc37460

nicoloboschi force-pushed the crd-parallel branch from 4d112ac to cc37460 Compare January 27, 2023 08:19

manusa approved these changes Jan 27, 2023

View reviewed changes

manusa added this to the 6.5.0 milestone Jan 27, 2023

manusa merged commit a1f6505 into fabric8io:master Jan 28, 2023

nicoloboschi deleted the crd-parallel branch January 31, 2023 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve: generate crds in parallel and optimize code #4644

improve: generate crds in parallel and optimize code #4644

nicoloboschi commented Dec 5, 2022 •

edited by manusa

Loading

andreaTP commented Dec 5, 2022

andreaTP commented Dec 6, 2022

nicoloboschi commented Dec 6, 2022

iocanel Dec 12, 2022

nicoloboschi Dec 13, 2022 •

edited

Loading

iocanel left a comment

sonarcloud bot commented Dec 14, 2022

nicoloboschi commented Dec 22, 2022

nicoloboschi commented Jan 14, 2023

andreaTP commented Jan 16, 2023

manusa commented Jan 17, 2023

nicoloboschi commented Jan 17, 2023

andreaTP commented Jan 17, 2023

nicoloboschi commented Jan 17, 2023

andreaTP commented Jan 17, 2023

nicoloboschi commented Jan 18, 2023

andreaTP commented Jan 18, 2023

metacosm left a comment

nicoloboschi commented Jan 26, 2023

gastaldi left a comment

gastaldi Jan 26, 2023

gastaldi Jan 26, 2023

nicoloboschi Jan 27, 2023

nicoloboschi commented Jan 27, 2023

manusa left a comment

sonarcloud bot commented Jan 27, 2023

improve: generate crds in parallel and optimize code #4644

improve: generate crds in parallel and optimize code #4644

Conversation

nicoloboschi commented Dec 5, 2022 • edited by manusa Loading

Description

Type of change

Checklist

andreaTP commented Dec 5, 2022

andreaTP commented Dec 6, 2022

nicoloboschi commented Dec 6, 2022

iocanel Dec 12, 2022

Choose a reason for hiding this comment

nicoloboschi Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

iocanel left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 14, 2022

nicoloboschi commented Dec 22, 2022

nicoloboschi commented Jan 14, 2023

andreaTP commented Jan 16, 2023

manusa commented Jan 17, 2023

nicoloboschi commented Jan 17, 2023

andreaTP commented Jan 17, 2023

nicoloboschi commented Jan 17, 2023

andreaTP commented Jan 17, 2023

nicoloboschi commented Jan 18, 2023

andreaTP commented Jan 18, 2023

metacosm left a comment

Choose a reason for hiding this comment

nicoloboschi commented Jan 26, 2023

gastaldi left a comment

Choose a reason for hiding this comment

gastaldi Jan 26, 2023

Choose a reason for hiding this comment

gastaldi Jan 26, 2023

Choose a reason for hiding this comment

nicoloboschi Jan 27, 2023

Choose a reason for hiding this comment

nicoloboschi commented Jan 27, 2023

manusa left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 27, 2023

nicoloboschi commented Dec 5, 2022 •

edited by manusa

Loading

nicoloboschi Dec 13, 2022 •

edited

Loading