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.

Sign up for GitHub

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

Jump to bottom

[java-generator] performance improvementes #4867

Merged
merged 10 commits into from
Feb 14, 2023
Merged

[java-generator] performance improvementes #4867

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6ca4012
[java-generator] performance improvementes
mariofusco Feb 10, 2023
9745e19
remove unused imports
mariofusco Feb 10, 2023
5bb577a
wip
mariofusco Feb 10, 2023
cfc7f3d
wip
mariofusco Feb 10, 2023
2cc3928
add benchmark
mariofusco Feb 13, 2023
a8b6ead
wip
mariofusco Feb 13, 2023
62141fa
code format
mariofusco Feb 14, 2023
6d9b466
fix test
mariofusco Feb 14, 2023
067784b
fix test
mariofusco Feb 14, 2023
6b5b279
add missing headers
mariofusco Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 41 additions & 21 deletions java-generator/core/src/main/java/io/fabric8/java/generator/FileJavaGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.CompletableFuture;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static io.fabric8.java.generator.CRGeneratorRunner.groupToPackage;
Expand Down Expand Up @@ -83,32 +85,50 @@ private void runOnSingleSource(File source, File basePath) {
resources.add(deserialized);
}

resources.parallelStream()
.forEach(
rawResource -> {
if (rawResource != null && rawResource instanceof HasMetadata) {
final HasMetadata resource = (HasMetadata) rawResource;
writeCRCompilationUnits(basePath, generateWritableCRCompilationUnits(resources));

if (resource != null && resource.getKind()
.toLowerCase(Locale.ROOT)
.equals("customresourcedefinition")) {
CustomResourceDefinition crd = (CustomResourceDefinition) resource;

final String basePackage = groupToPackage(crd.getSpec().getGroup());

crGeneratorRunner.generate(crd, basePackage).parallelStream()
.forEach(w -> w.writeAllJavaClasses(basePath));
} else {
LOGGER.warn("Not generating nothing for resource of kind: {}", resource.getKind());
}
} else {
LOGGER.warn("Not generating nothing for unrecognized resource: {}", Serialization.asYaml(rawResource));
}
});
} catch (FileNotFoundException e) {
throw new JavaGeneratorException("File " + source.getAbsolutePath() + " not found", e);
} catch (IOException e) {
throw new JavaGeneratorException("Exception reading " + source.getAbsolutePath(), e);
}
}

private List<WritableCRCompilationUnit> generateWritableCRCompilationUnits(List<Object> resources) {
return resources.parallelStream()
.flatMap(
rawResource -> {
if (rawResource != null && rawResource instanceof HasMetadata) {
final HasMetadata resource = (HasMetadata) rawResource;

if (resource != null && resource.getKind()
.toLowerCase(Locale.ROOT)
.equals("customresourcedefinition")) {
CustomResourceDefinition crd = (CustomResourceDefinition) resource;

final String basePackage = groupToPackage(crd.getSpec().getGroup());
return crGeneratorRunner.generate(crd, basePackage).stream();
} else {
LOGGER.warn("Not generating nothing for resource of kind: {}", resource.getKind());
}
} else {
LOGGER.warn("Not generating nothing for unrecognized resource: {}", Serialization.asYaml(rawResource));
}
return Stream.empty();
})
.collect(Collectors.toList());
}

private void writeCRCompilationUnits(File basePath, List<WritableCRCompilationUnit> wCUs) {
CompletableFuture<Void>[] futures = new CompletableFuture[wCUs.size()];

int i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick] can you, please, move wCUs.size() to a val (e.g. total) and reuse it as a boundary in the following for loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for (WritableCRCompilationUnit w : wCUs) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (WritableCRCompilationUnit w : wCUs) {
for (int i = 0; i < total; i++) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

futures[i++] = CompletableFuture.runAsync(() -> w.writeAllJavaClasses(basePath));
Copy link
Member

Choose a reason for hiding this comment

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

Have you attempted to use a different Thread Pool than the ForkJoinPool.commonPool since it's used also by the underlying parallelStreams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At that point the code generation performed in the parallel stream is finished and that pool should be completely available to write files. Also consider that creating a brand new thread pool could be very likely much more expansive than writing the file, that's why imo it is a better idea to reuse an already existing pool. Anyway if we had the JMH benchmark that I'm proposing in the other comment we could properly measure this.

Copy link
Member

Choose a reason for hiding this comment

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

You can find the yaml I was using here: https://github.com/kserve/kserve/releases/download/v0.10.0/kserve.yaml
feel free to add it to the CompilationTest along with this PR.

Feel free to add a JMH benchmark if you think that it's useful, but I'm convinced that this PR is perfectly enough for a first iteration.
In case performance issues becomes a problem we can revisit the implementation later on 👍

Thanks again!

}

for (int j = 0; j < futures.length; j++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int j = 0; j < futures.length; j++) {
for (int j = 0; j < total; j++) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

futures[j].join();
}
}
}
5 changes: 2 additions & 3 deletions java-generator/core/src/main/java/io/fabric8/java/generator/WritableCRCompilationUnit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,14 @@ public void writeAllJavaClasses(File basePath) {
try {
createFolders(basePackage, basePath);
for (GeneratorResult.ClassResult cr : this.classResults) {
String pkg = cr.getCompilationUnit()
.getPackageDeclaration()
String pkg = cr.getPackageDeclaration()
.map(NodeWithName::getNameAsString)
.orElse(null);
File path = createFolders(pkg, basePath);

writeToFile(
path.toPath().resolve(cr.getName() + ".java").toFile(),
cr.getCompilationUnit().toString());
cr.getJavaSource());
}
} catch (Exception e) {
throw new JavaGeneratorException(e);
Expand Down
28 changes: 23 additions & 5 deletions java-generator/core/src/main/java/io/fabric8/java/generator/nodes/GeneratorResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,45 @@
package io.fabric8.java.generator.nodes;

import com.github.javaparser.ast.CompilationUnit;
import com.github.javaparser.ast.PackageDeclaration;
import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
import com.github.javaparser.ast.body.EnumDeclaration;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

public class GeneratorResult {

public static class ClassResult {
private final String name;
private final CompilationUnit cu;
private final String javaSource;

public ClassResult(String name, CompilationUnit cu) {
this.name = name;
this.cu = cu;
this.javaSource = cu.toString();
}

public String getName() {
return name;
}

public CompilationUnit getCompilationUnit() {
return cu;
public String getJavaSource() {
return javaSource;
}

public ClassResult(String name, CompilationUnit cu) {
this.name = name;
this.cu = cu;
public Optional<PackageDeclaration> getPackageDeclaration() {
return cu.getPackageDeclaration();
}

public Optional<EnumDeclaration> getEnumByName(String enumName) {
return cu.getEnumByName(enumName);
}

public Optional<ClassOrInterfaceDeclaration> getClassByName(String className) {
return cu.getClassByName(className);
}
}

Expand Down
3 changes: 2 additions & 1 deletion java-generator/core/src/main/java/io/fabric8/java/generator/nodes/JCRObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package io.fabric8.java.generator.nodes;

import com.github.javaparser.ast.CompilationUnit;
import com.github.javaparser.ast.PackageDeclaration;
import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
import com.github.javaparser.ast.expr.Name;
import com.github.javaparser.ast.expr.NameExpr;
Expand Down Expand Up @@ -86,7 +87,7 @@ public String getType() {
public GeneratorResult generateJava() {
CompilationUnit cu = new CompilationUnit();
if (!pkg.isEmpty()) {
cu.setPackageDeclaration(pkg);
cu.setPackageDeclaration(new PackageDeclaration(new Name(pkg)));
}
ClassOrInterfaceDeclaration clz = cu.addClass(className);

Expand Down
31 changes: 19 additions & 12 deletions java-generator/core/src/main/java/io/fabric8/java/generator/nodes/JObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
package io.fabric8.java.generator.nodes;

import com.fasterxml.jackson.databind.JsonNode;
import com.github.javaparser.StaticJavaParser;
import com.github.javaparser.ast.CompilationUnit;
import com.github.javaparser.ast.Modifier;
import com.github.javaparser.ast.NodeList;
import com.github.javaparser.ast.PackageDeclaration;
import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
import com.github.javaparser.ast.body.EnumDeclaration;
import com.github.javaparser.ast.body.FieldDeclaration;
Expand Down Expand Up @@ -128,7 +130,7 @@ private String getSortedFieldsAsParam(Set<String> list) {
public GeneratorResult generateJava() {
CompilationUnit cu = new CompilationUnit();
if (!this.pkg.isEmpty()) {
cu.setPackageDeclaration(this.pkg);
cu.setPackageDeclaration(new PackageDeclaration(new Name(this.pkg)));
}
ClassOrInterfaceDeclaration clz = cu.addClass(this.className);

Expand Down Expand Up @@ -156,7 +158,7 @@ public GeneratorResult generateJava() {
addExtraAnnotations(clz);
}

clz.addImplementedType("io.fabric8.kubernetes.api.model.KubernetesResource");
clz.addImplementedType( new ClassOrInterfaceType(null, "io.fabric8.kubernetes.api.model.KubernetesResource") );

List<GeneratorResult.ClassResult> buffer = new ArrayList<>(this.fields.size() + 1);

Expand All @@ -171,7 +173,7 @@ public GeneratorResult generateJava() {
boolean isEnum = !gr.getInnerClasses().isEmpty();
if (isEnum) {
for (GeneratorResult.ClassResult enumCR : gr.getInnerClasses()) {
Optional<EnumDeclaration> ed = enumCR.getCompilationUnit().getEnumByName(enumCR.getName());
Optional<EnumDeclaration> ed = enumCR.getEnumByName(enumCR.getName());
if (ed.isPresent()) {
clz.addMember(ed.get());
}
Expand All @@ -184,14 +186,14 @@ public GeneratorResult generateJava() {
String fieldType = prop.getType();

try {
FieldDeclaration objField = clz.addField(fieldType, fieldName, Modifier.Keyword.PRIVATE);
FieldDeclaration objField = clz.addField(toClassOrInterfaceType(fieldType), fieldName, Modifier.Keyword.PRIVATE);
objField.addAnnotation(
new SingleMemberAnnotationExpr(
new Name("com.fasterxml.jackson.annotation.JsonProperty"),
new StringLiteralExpr(originalFieldName)));

if (isRequired) {
objField.addAnnotation("io.fabric8.generator.annotation.Required");
objField.addAnnotation( "io.fabric8.generator.annotation.Required");
}
if (prop.getMaximum() != null) {
objField.addAnnotation(
Expand Down Expand Up @@ -241,7 +243,7 @@ public GeneratorResult generateJava() {
new SingleMemberAnnotationExpr(
new Name("com.fasterxml.jackson.annotation.JsonSetter"),
new NameExpr("nulls = com.fasterxml.jackson.annotation.Nulls.SET")));
objField.addAnnotation("io.fabric8.generator.annotation.Nullable");
objField.addAnnotation(new NormalAnnotationExpr(new Name("io.fabric8.generator.annotation.Nullable"), new NodeList<>()));
}

if (prop.getDefaultValue() != null) {
Expand Down Expand Up @@ -280,15 +282,15 @@ public GeneratorResult generateJava() {
.setType(mapType)
.setInitializer("new java.util.HashMap<>()")));

objField.addAnnotation("com.fasterxml.jackson.annotation.JsonIgnore");
objField.addAnnotation(new NormalAnnotationExpr(new Name("com.fasterxml.jackson.annotation.JsonIgnore"), new NodeList<>()));

objField.createGetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnyGetter");
objField.createSetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnySetter");
objField.createGetter().addAnnotation(new NormalAnnotationExpr(new Name("com.fasterxml.jackson.annotation.JsonAnyGetter"), new NodeList<>()));
objField.createSetter().addAnnotation(new NormalAnnotationExpr(new Name("com.fasterxml.jackson.annotation.JsonAnySetter"), new NodeList<>()));

MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperty", Modifier.Keyword.PUBLIC);
additionalSetter.addAnnotation("com.fasterxml.jackson.annotation.JsonAnySetter");
additionalSetter.addParameter("String", "key");
additionalSetter.addParameter("Object", "value");
additionalSetter.addAnnotation(new NormalAnnotationExpr(new Name("com.fasterxml.jackson.annotation.JsonAnySetter"), new NodeList<>()));
additionalSetter.addParameter(new ClassOrInterfaceType(null, "java.lang.String"), "key");
additionalSetter.addParameter(new ClassOrInterfaceType(null, "java.lang.Object"), "value");
additionalSetter
.setBody(new BlockStmt().addStatement(new NameExpr("this." + Keywords.ADDITIONAL_PROPERTIES + ".put(key, value)")));
}
Expand Down Expand Up @@ -320,4 +322,9 @@ private Expression generatePrimitiveDefaultInitializerExpression(AbstractJSONSch
return null;
}
}

static ClassOrInterfaceType toClassOrInterfaceType( String className ) {
String withoutDollars = className.replace("$", "."); // nested class in Java cannot be used in casts
return withoutDollars.indexOf('<') >= 0 ? StaticJavaParser.parseClassOrInterfaceType(withoutDollars) : new ClassOrInterfaceType(null, withoutDollars);
}
}
2 changes: 1 addition & 1 deletion java-generator/core/src/test/java/io/fabric8/java/generator/ApprovalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,6 @@ private CustomResourceDefinition getCRD(String name) {

private String getJavaClass(List<GeneratorResult.ClassResult> classResults, String name) {
GeneratorResult.ClassResult cr = classResults.stream().filter(c -> c.getName().equals(name)).findFirst().get();
return cr.getCompilationUnit().toString();
return cr.getJavaSource();
}
}
Loading