Rewrite JarResultBuildStep and enable parallel compression of jars #49585
Conversation
quarkus-bot
bot
added
area/core
area/devtools
| <!-- let's avoid having commons-io crawling into quarkus-core --> | ||
| <exclude>commons-io:commons-io</exclude> |
There was a problem hiding this comment.
I will ban it through ForbiddenAPIs instead but we have some unrelated code depending on it so I need to clean up this code before doing it.
Will open a follow-up PR once this one is in.
There was a problem hiding this comment.
Actually, this is fine, we already have a local forbidden-apis rule for core/deployment banning usage of Commons IO.
We are using it in a lot of test utils everywhere so it's hard to fully get rid of it. We could get rid of most of them but the IOUtils dependencies to get the content of a URL are handy.
| }; | ||
| } | ||
|
|
||
| private static ExecutorService initExecutorService() { |
There was a problem hiding this comment.
It feels quite wasteful to create a whole new Executor for each single jar?
What about reusing the build executor, and wrapping it into an adaptor wich would ignore the shutdown request issues by commons-compress?
There was a problem hiding this comment.
Oh, that you are plain right. I will create only one. BUT I prefer avoiding reusing the build executor as really I don't want to make assumptions as to what Commons Compress is doing with the executor.
I know it's a bit suboptimal but I think it's safer.
There was a problem hiding this comment.
I had a look and I think I will keep it as it is even if suboptimal. The code in Commons Compress has several comments saying they absolutely want the executor to be shut down.
I know it's probably being a bit too safe but I really prefer not breaking their contract.
There was a problem hiding this comment.
but that's a lot of threads? And they wouldn't coordinate well with other jobs running on the existing executors - it worries me that it's not "a little suboptimal", might lead to serious problems like hard to reproduce issues, unmanaged spikes of memory
There was a problem hiding this comment.
has several comments saying they absolutely want the executor to be shut down.
I know it's probably being a bit too safe but I really prefer not breaking their contract.
Ok, I see - they probably do some dodgy things then - I guess you're right in being cautious. What about at least reusing the ParallelScatterZipCreator across your various needs, would that be an option?
There was a problem hiding this comment.
So I ended up doing it... and the result is underwhelming. It's quite slower.
I thought it could be due to the fact that we end up starting too many threads so I tried your patch here: #49575 .
And basically:
- your patch there makes the global build faster at 2 * cores
- but the specific jar compression is faster at 4 * cores (but the global build is slower even with the compression being faster)
So I think we are looking at something that specifically requires its own thread pool with specific configuration.
I'm going to try with your patch + executor decoupling but only one Executor Service for all the jars.
There was a problem hiding this comment.
Mkay, I can't actually reproduce my numbers...
I'll push the patch reusing the common thread pool...
|
It's interesting that you opted to parallelize the writing of a single jar - wasn't there also some opportunities to write different jars in parallel? Would that be a different optimisation that we could do as well ? But all such tweaks would benefit the most from actually sharing executors. |
We could at some point, but I needed to optimize the writing of a single jar anyway as not all jars are created equals. Some of them are very large, some of them very small. I'm not convinced adding parallelization of the whole jar operation after this work would bring some benefits. Feel free to have fun with it though :). |
It doesn't feel like a priority now that you went with the nuclear approach :) But I do wonder how efficient "parallel compression" in commons-compress is, compared to a "simple" zipstream for each jar. |
This comment has been minimized.
This comment has been minimized.
|
FWIW, the failures are due to some Maven tests using very old versions of Commons IO as a dependency, and not relying on our BOM. As for Gradle, the problem was that a test tests the exact list of files in |
In some cases, we build only one jar (native image and legacy thin jar but only the first one is relevant this day). So just parallelizing the build of each jar won't help. In the case where we build the most jars, it's like 3. One of the three So just parallelizing building each jar won't bring you much. |
|
OK, it should be fine now. |
This comment has been minimized.
This comment has been minimized.
|
@geoand this is another for you when you're back from PTO. Sorry :). |
This class had become completely unmanageable due to its size. Given I'm willing to invest some time to see if we can improve our zip build time, this is a necessary step in preparation to the upcoming improvements.
Including the native image source jar. It is based on commons-compress. I haven't implemented parallel jar build for uberjars for now. This is a known limitation.
I would have squashed it but it's not easy to squash into initial commit due to conflicts.
Status for workflow
|
|
@gsmet FYI, this looks like it caused the following issue in Quarkus LangChain4j Not a problem, just wanted to raise awareness that there might be other extensions that could fail as well |
Note
This is part of my efforts on large monoliths but it should benefit all applications in the end.
This is a big patch, sorry, but I tried to have semantic commits, even if the first commit is quite large by itself.
My goal was to only implement parallel compression of jars using Commons Compress... but I ended up being unable to do it with
JarResultBuildStepin its current state: the class was too big (1600+ lines) and it was too hard to actually comprehend the specificities of each format.Thus why my first step was to rewrite this class with a proper hierarchy and each format splitted. FWIW, it's not the first time I struggled to adjust things there so my personal opinion is that this rewrite was long overdue.
Then I extracted the creation of the archive to an interface and finally implemented parallel compression using Commons Compress.
The last commit is just some final cleanup that was too entangled to actually be squashed in one of the existing commits.
For my test app with 37k Java source classes (+ all the ones we generate), it went from
6 secondsto1.6 seconds, so a3.75xspeedup.For now I'm still using the
ZipFileSystemapproach for Uberjars. Uberjars are specific as the Manifest is potentially updated at the end to include the multi release bits and it comes with some subtleties as the Manifest has to be added first to the jar, nothing insurmountable but this work already consumes a lot of bandwith. I could be convinced to put the extra work and get rid of it at some point. Not in this PR though.