Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
> We'll need some sort of resource manager (no matter how crude) in rpm sooner > than later to handle this sort of stuff, but in the One simple possible solution can be using a semaphore that will be respected by a compression library. `zstd` issue about it lives here: https://github.com/facebook/zstd/issues/1097 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-680826776___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Merged #1324 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#event-3669884791___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Okay, the first commit could've used a more elaborate commit message but I what I missed I missed. I do blame the GH interface to some extent on that though :( Thanks for the patches! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-676017735___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@marxin commented on this pull request. > @@ -523,6 +523,27 @@ fprintf(stderr, "*** ufdOpen(%s,0x%x,0%o)\n", url, > (unsigned)flags, (unsigned)mo return fd; } +/* Return number of threads ought to be used for compression based + on a parsed value threads (e.g. from w7T0.xzdio or w7T16.xzdio). + Value -1 means automatic detection. */ + +static int +get_compression_threads (int threads) Sure. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#discussion_r472862960___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@marxin pushed 2 commits. 2c659f7ec7dea6fb1a4a25d13a91592c0acb7d97 Add get_compression_threads and refactor code. c771b3b43d05d76d0b19df9a2c25eba3c9f1e6a1 Enable thread autodetection for a parallel compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324/files/b490f59832a056a897cd1835d7faed4a41585327..c771b3b43d05d76d0b19df9a2c25eba3c9f1e6a1 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request. > @@ -523,6 +523,27 @@ fprintf(stderr, "*** ufdOpen(%s,0x%x,0%o)\n", url, > (unsigned)flags, (unsigned)mo return fd; } +/* Return number of threads ought to be used for compression based + on a parsed value threads (e.g. from w7T0.xzdio or w7T16.xzdio). + Value -1 means automatic detection. */ + +static int +get_compression_threads (int threads) Extra space before ( here too, apologies for missing previously. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470232032___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@marxin pushed 2 commits. 854bf2775fbaaa7b0621de46f3e42a165a5fd60c Add get_compression_threads and refactor code. b490f59832a056a897cd1835d7faed4a41585327 Enable thread autodetection for a parallel compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324/files/8480318844c3d88c1752ae0e951ba06cf0fe1b50..b490f59832a056a897cd1835d7faed4a41585327 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Oops sorry, closing was accidental. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-675888299___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Reopened #1324. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#event-3669326724___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Closed #1324. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#event-3669326500___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Yeah, overall looks like intended now, just minor nits/cosmetics. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-675888024___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request. > @@ -1143,9 +1132,11 @@ static rpmzstd rpmzstdNew(int fdno, const char *fmode) goto err; } - if (threads > 0) + threads = get_compression_threads (threads); As noted above, there's an extra space before (threads) here. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470169897___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request.
> @@ -797,8 +822,7 @@ static LZFILE *lzopen_internal(const char *mode, int fd,
> int xz)
ret = lzma_easy_encoder(>strm, level,
LZMA_CHECK_SHA256);
#ifdef HAVE_LZMA_MT
} else {
- if (threads == -1)
- threads = rpmExpandNumeric("%{getncpus}");
+ threads = get_compression_threads (threads);
There's an extra space before (threads), both here and in the other
get_compression_threads() call.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470168590___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request.
> @@ -1143,9 +1132,11 @@ static rpmzstd rpmzstdNew(int fdno, const char *fmode)
goto err;
}
- if (threads > 0)
+ threads = get_compression_threads (threads);
+ if (threads > 0) {
if (ZSTD_isError (ZSTD_CCtx_setParameter(_stream, ZSTD_c_nbWorkers,
threads)))
rpmlog(RPMLOG_WARNING, "zstd library does not support
multi-threading\n");
Not introduced in this commit of course, but while in the neighborhood... This
should really either be RPMLOG_DEBUG or marked for translation, and I think
RPMLOG_DEBUG is the way to go, just to avoid endless, useless warning spew.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470167841___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request.
> @@ -523,6 +523,28 @@ fprintf(stderr, "*** ufdOpen(%s,0x%x,0%o)\n", url,
> (unsigned)flags, (unsigned)mo
return fd;
}
+/* Return number of threads ought to be used for compression based
+ on a parsed value threads (e.g. from w7T0.xzdio or w7T16.xzdio).
+ Value -1 means automatic detection. */
+
+static int
+get_compression_threads (int threads)
+{
+if (threads == -1)
+ threads = rpmExpandNumeric("%{getncpus}");
+
+#if __WORDSIZE == 32
+/* Limit thread up to 4 for 32-bit systems. */
+if (threads > 4)
+{
{ belongs on the if-line in rpm coding style. The "thread" in the comment
should probably be in plural.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470166476___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai commented on this pull request.
> +/* Return number of threads ought to be used for compression based
+ on a parsed value threads (e.g. from w7T0.xzdio or w7T16.xzdio).
+ Value -1 means automatic detection. */
+
+static int
+get_compression_threads (int threads)
+{
+if (threads == -1)
+ threads = rpmExpandNumeric("%{getncpus}");
+
+#if __WORDSIZE == 32
+/* Limit thread up to 4 for 32-bit systems. */
+if (threads > 4)
+{
+ threads = 4;
+ rpmlog(RPMLOG_WARNING, "threading compression limited to 4 threads on
32-bit systems\n");
I think this should be RPMLOG_DEBUG instead to avoid spewage on systems where
it actually gets activated.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324#pullrequestreview-470165744___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@pmatilai I hope I addressed all your comments. Feel free to look at it now.. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-672804296___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Mm, that's not quite what I meant: xz and zstd should use a common helper function for the thread number autodetection, and that function offers a nice place to restrict the number on 32bit architectures centrally (for rpmio purposes until we get something better). OTOH, didn't realize XZ has this memlimit option too, which we probably shouldn't discard as it could be necessary on 64bit arch too I guess. I'm also starting to have second thoughts about completely disabling threading on 32bit - we cap threads to maximum of four in build-code (which sometimes is too much however) so why do something else here? Though, what this really needs is a central place for it all instead of these local tweaks. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-672749708___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
> I think it should at least follow the same convention as XZ: T with no > numbers mean autodetection (its okay if 0 means that too). This seems to > silently change XZ behavior too which is not okay. I'm going to update the documentation of my change. Yes, it was intentional to sync behavior of `T0` and `T`, as `zstd` and `xz` use `-T0` in order to autodetect number of threads. > We'll need some sort of resource manager (no matter how crude) in rpm sooner > than later to handle this sort of stuff, but in the meanwhile I think a first > step towards sanity could be axing the crazily complicated logic from XZ and > simply limit threading to 64bit platforms. Works for me. > And with that, it should be possible to refactor this to use common > thread-number autodetection code between xzstd and the XZ case. I would like to. Currently, `xz` uses the memory limit. That functionality is missing for `zstd`, but as said, it does not work for concurrent creation of `rpm` files. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-670848352___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
I think it should at least follow the same convention as XZ: T with no numbers mean autodetection (its okay if 0 means that too). This seems to silently change XZ behavior too which is not okay. We'll need some sort of resource manager (no matter how crude) in rpm sooner than later to handle this sort of stuff, but in the meanwhile I think a first step towards sanity could be axing the crazily complicated logic from XZ and simply limit threading to 64bit platforms. And with that, it should be possible to refactor this to use common logic for this and the XZ case. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-668558057___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
> Right, this isn't any worse than what we already have for XZ, but then again > what is there for XZ is simply wrong in the face of concurrent package > generation as we have now. The lowly IO stream has no way of knowing how many > threads it can legitimately consume. That said, would it be possible to merge my pull request? I'm planning to experiment with `T0` when building `openSUSE:Factory`. Or do you see a way how to communicate the top-level parallelism to the compressing API? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-668488681___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Right, this isn't any worse than what we already have for XZ, but then again what is there for XZ is simply wrong in the face of concurrent package generation as we have now. The lowly IO stream has no way of knowing how many threads it can legitimately consume. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-668022408___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
Thanks for the patch. Unfortunately auto-detecting the number of threads is a
more complicated topic and any solution for zstd needs to have a look at the
larger picture. We added support for building multiple package in parallel, so
the threads need to be split between those concurrent runs. While relying on
`%{getncpus}` is probably the right thing to do, it does not yet contain a
reliable enough value. There are some freaky architectures and machines out
there that need better policies than just counting CPUs. Especially large 32
bit machines (arm, z-series) that have a lot of CPUs can run out of address
space pretty quickly.
I still think this patch is a good starting point but expect it to not go in
until those other issues are ironed out.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-667092610___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
@marxin pushed 1 commit. 34a78ee61a67a9ffe11142c045fd1fa01e44160e Enable thread autodetection for a parallel compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324/files/cd31048911a1d3c6094e4530f5e7e4975849be4a..34a78ee61a67a9ffe11142c045fd1fa01e44160e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
It's a follow up of #1303, so adding @ignatenkobrain as a reviewer ;) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1324#issuecomment-666359866___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Enable thread autodetection for a parallel compression. (#1324)
When using xz or zstd, it can be handy to automatically detect
number of threads. We can use `%{getncpus}` for that.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1324
-- Commit Summary --
* Enable thread autodetection for a parallel compression.
-- File Changes --
M macros.in (1)
M rpmio/rpmio.c (14)
-- Patch Links --
https://github.com/rpm-software-management/rpm/pull/1324.patch
https://github.com/rpm-software-management/rpm/pull/1324.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1324
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
