Run binary package generation via thread pools #226
Conversation
|
cool! I'll try get around reviewing it myself at least when I find the time, then provide feedback and/or my approval of (FWIW). :) |
build/pack.c
Outdated
| @@ -14,6 +14,8 @@ | |||
| #include <rpm/rpmfileutil.h> | |||
| #include <rpm/rpmlog.h> | |||
|
|
|||
| #include <prtpool.h> /* NSPR thread pools */ | |||
There was a problem hiding this comment.
I'm kinda reluctant for such external dependencies
There was a problem hiding this comment.
rpm already has a hard dependency on nss, and nss has a hard dependency on nspr, so it seemed obvious to utilize the thread pool API from nspr. Why you are reluctant?
There was a problem hiding this comment.
@kanavin NSS is not hard dependency, it has support for openssl and other crypto backend and I would say that NSS is hardest to bootstrap so we swithced to OpenSSL in Fedora not so long time ago... So assuming NSS to be present is very bad idea.
There was a problem hiding this comment.
It'd be pretty hard for me to compile this on Mac OS now, as the Netscape stuff is difficult to bootstrap.
There was a problem hiding this comment.
Right, openssl support is very recent, so I missed that. Do you have suggestions on what could provide a good thread pool API, if not nspr? I wouldn't want to do a poor internal re-implementation on top of pthreads.
The whole thing can also be made conditional.
There was a problem hiding this comment.
openmp, unless for some reason would turn out to be unfeasiable, would be the definitive candidate IMO given it's standardization and implementation available from all compilers of relevance. 👍
There was a problem hiding this comment.
Alright, I'll rework the patches with openmp, and re-do the pull request.
|
There you go, I force pushed the reworked patches. It is 100% pure openmp now :) |
|
You even caught some design shortcomings of rpm. and improved on poor implementations, bravo! While only having quickly reviewed code, (but having knowledge of openmp and code in question at leastt), only thing I can come to think of is more aestetics, ie.
would be more readable IMO. But wars can't possible have started by less, so I give my approval after giving a first sight. 👍 :) |
|
The reason I didn't change the indentation when wrapping the code in those pragmas was to keep the patch more readable. Also, don't merge these just yet: they work flawlessly on my machine, but I get reports that they crash on our automated build system. It may be an issue with an older gcc version. Obviously I'd like you to try them and report :) EDIT: I also broke the last patch when rebasing it to master, sorry about that. So look at the code, but don't try to build :) |
|
I have now updated the patchset again, addressing the crashes with old gcc, and Jeff's comment above. And this time they should be rebased correctly :) |
|
I've no prior knowledge of OMP but doesn't look half bad on first sight. Cosmetics aside, we can't really have half the codebase doing half-assed manual pthread locking here and there and another half using OMP, AIUI this is undefined behavior and the string pool is heavily used by non-build code as well. So to set expectations straight: we'll look at merging after rpm 4.14 is branched off, which will be at least couple of months away still. |
|
This seems to work correctly on my Mac OS X 10.10 system, though admittedly I don't have a great test case with lots of subpackages. With ~3 subpackages (progs, libs, devel), it seems to work fine. |
|
BTW it'd probably be easier and less intrusive to make buildtime and buildhost part of the spec struct, initialized in newSpec() or so. AFAICS all the relevant places are receiving spec as the argument already, with the exception of writeRPM(). |
|
@kanavin any news here? |
|
@ignatenkobrain The patches have been merged to Yocto project: As @pmatilai said he will not look at them until after 4.14 is ready (which is two months away), right now I am not in a particular hurry to rebase and update them. But I haven't forgotten :) Is there anything specific you would like me to do now? |
|
@kanavin Now that rpm 4.14 has been branched, could you rebase them for review to merge into master? |
So that it can be run as a thread pool task.
Otherwise multithreaded rpm building explodes in various ways due to data races.
|
@Conan-Kudo @ignatenkobrain I have rebased this against master now. I checked that it builds, but did not run it against any spec files. |
|
let me reopen PR to run CI tests |
…uildTime() Their use is causing difficult to diagnoze data races when building multiple packages in parallel, and is a bad idea in general, as it also makes it more difficult to reason about code.
spec.c: In function 'getBuildTime':
spec.c:214:9: error: 'errno' undeclared (first use in this function); did you mean 'h_errno'?
errno = 0;
^~~~~
h_errno |
|
@ignatenkobrain Sorry, forgot to push the fix. Now fixed. |
|
I will try to build texlive with and without PR and will report results :) |
|
P.S. it has thousands of subpackages |
not sure if we should print those messages... |
|
It would be nice to run dependency generators in parallel as well (although it might disallow us in future to do some advanced generators which would take subpackages into account, but AFAIK no one is planning to work on something like this in foreseeable future so doesn't seem to be a problem). This is what taking quite a lot of time for some of subpackages, so if they would run in parallel, this would speedup build. ResultsBeforeAfterHardwareCommentsIt didn't really seem to generate binary packages in parallel, but probably it worked, not sure ;) |
|
@ignatenkobrain The reason we see a lot of speed up in Yocto with this patchset is that building the sources happens outside of rpm, and we only use rpm to do packaging of binaries. If the bulk of the above time is spend building texlive from source then of course the final difference won't be as much. Can you investigate a little bit where the time goes? |
|
@kanavin, even 8 minutes speedup is good ;) |
|
Shouldn't the time have come to review this again for finally merging this one after more than a half year now? With rpm 4.14 out, it should be the right time to merge this one now, no? :) |
|
Ok, I pushed the first and the last patch as a sign of good will. |
|
@ignatenkobrain testing your patches in the Yocto project (with some extra debugging statements) on openSUSE's open build service instance its clear that the packages are still being built sequentially I'm not sure why yet, but here's some debug output below. Some of us at SUSE are quite keen to get this working and I have time allocated to it primarily because it should speed up our kernel builds significantly but also because it will reduce the load on our build service significantly.
|
|
Never mind it seems that the patch is not being applied correctly here |
|
Is it possible to get this accepted? I've been testing it on openSUSE's build services, with some very rough (semi controlled) benchmarking the speedup's aren't huge a medium size package that splits out into multiple packages might save 10 seconds off a 5-6 minute build but when you take that across a full distro rebuild of 10,000+ packages it will be significant. Then there are things like libreoffice which is a silly long build, where the building the binary process used to take 900 seconds and is now cut down to 125 seconds, which equates to around an 8 minutes quicker although in the context of a almost 4 hr build time. I am now looking into making a similar change for the processing binaries stage. |
The shared string pool is in a very central role in several operations, it's kinda embarrasing that we haven't had any thread protection on it. Not that anybody has asked either, prior to coming up as part of #226 (to enable threaded package creation). Test-suite and couple of smoke-tests with the #226 pass, but only lightly tested. Then again, it's relatively straightforward. As a general rule, locks are taken on all exported interfaces on entry and released on exit, internal callers never lock anything. In rpm usage at least, performance hit seems negligible.
|
FWIW, thread protection added to the string pool as of commit 7ffc4d1 so that part of these patches can be dropped. Brief testing showed no problems with the rest applied, but I don't have any real testcase for parallelism at hand. |
It's worth noting that this really is walking on thin ice as only few parts of rpm are thread-protected. The spec is entirely unprotected so must be accessed only for read-only purposes from parallel jobs (and we should work towards enforcing that via const-correctness and other protection as needed), and similarly the package struct and headers are unprotected so they can only be manipulated within a single thread. Based on initial work by Alexander Kanavin in PR rpm-software-management#226
|
For interested parties, I submitted a more elaborate version as #695 with further code cleanups and additional parallelization of file classifier. |
|
Yes, as already mentioned in an earlier comment to this PR: #226 (comment) |
It's worth noting that this really is walking on thin ice as only few parts of rpm are thread-protected. The spec is entirely unprotected so must be accessed only for read-only purposes from parallel jobs (and we should work towards enforcing that via const-correctness and other protection as needed), and similarly the package struct and headers are unprotected so they can only be manipulated within a single thread. Based on initial work by Alexander Kanavin in PR rpm-software-management#226
It's worth noting that this really is walking on thin ice as only few parts of rpm are thread-protected. The spec is entirely unprotected so must be accessed only for read-only purposes from parallel jobs (and we should work towards enforcing that via const-correctness and other protection as needed), and similarly the package struct and headers are unprotected so they can only be manipulated within a single thread. Based on initial work by Alexander Kanavin in PR rpm-software-management#226
|
As #695 supersedes this I am closing this PRs. |
It's worth noting that this really is walking on thin ice as only few parts of rpm are thread-protected. The spec is entirely unprotected so must be accessed only for read-only purposes from parallel jobs (and we should work towards enforcing that via const-correctness and other protection as needed), and similarly the package struct and headers are unprotected so they can only be manipulated within a single thread. Based on initial work by Alexander Kanavin in PR #226
Please see here for background information and rationale:
#211
Caveats: