[java-generator] performance improvementes #4867
Conversation
mariofusco
requested review from
manusa,
oscerd,
rohanKanojia,
shawkins and
sunix
as code owners
There was a problem hiding this comment.
Amazing job @mariofusco 🎉 🚀 🚀 🚀
Thanks a ton!!!
A minor style nitpick and it's good to go.
Can you share the perf difference before/after this change?
|
|
||
| int i = 0; | ||
| for (WritableCRCompilationUnit w : wCUs) { | ||
| futures[i++] = CompletableFuture.runAsync(() -> w.writeAllJavaClasses(basePath)); |
There was a problem hiding this comment.
Have you attempted to use a different Thread Pool than the ForkJoinPool.commonPool since it's used also by the underlying parallelStreams?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| private void writeCRCompilationUnits(File basePath, List<WritableCRCompilationUnit> wCUs) { | ||
| CompletableFuture<Void>[] futures = new CompletableFuture[wCUs.size()]; | ||
|
|
||
| int i = 0; |
There was a problem hiding this comment.
[nitpick] can you, please, move wCUs.size() to a val (e.g. total) and reuse it as a boundary in the following for loops?
| CompletableFuture<Void>[] futures = new CompletableFuture[wCUs.size()]; | ||
|
|
||
| int i = 0; | ||
| for (WritableCRCompilationUnit w : wCUs) { |
There was a problem hiding this comment.
| for (WritableCRCompilationUnit w : wCUs) { | |
| for (int i = 0; i < total; i++) { |
| futures[i++] = CompletableFuture.runAsync(() -> w.writeAllJavaClasses(basePath)); | ||
| } | ||
|
|
||
| for (int j = 0; j < futures.length; j++) { |
There was a problem hiding this comment.
| for (int j = 0; j < futures.length; j++) { | |
| for (int j = 0; j < total; j++) { |
In reality I can no longer run my main because I don't have anymore the yaml that I was using to test it. Is it possible to push it on main or somewhere else? That said I'm afraid that the code generation triggered by that file doesn't last long enough to see any reliably measurable difference. Maybe we could find a bigger test case and put it in a JMH benchmark? |
|
I added a JMH benchmark to demonstrate the performance improvement of this PR and got the following results. Before this PR: After |
|
SonarCloud Quality Gate failed. |
Fix #4865
This PR:
StaticJavaParser.parseClassOrInterfaceTypewith explicit javaparser AST nodes constructors