Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Sorry, this took a bit longer than it should have, Thanks for your work and patience! -- 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/1303#issuecomment-666324374___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Merged #1303 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/1303#event-3603495856___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
CI seems happy now, can please anybody merge it? I've got another patch based on this one that I would like to propose.. -- 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/1303#issuecomment-666188501___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> Looks like it fails to detect the right commit to checkout and build. Can you > force push a new version, please, to test if that get's it unstuck? Just do > some white space changes to the commit message. Done, good point! -- 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/1303#issuecomment-665651388___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. 790bd81bc215b7aea17cbc5eb2e7a53e15b99ded Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/11358ce35d0c98d716d68bb4824e86c44f78491b..790bd81bc215b7aea17cbc5eb2e7a53e15b99ded ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Looks like it fails to detect the right commit to checkout and build. Can you force push a new version, please, to test if that get's it unstuck? Just do some white space changes to the commit message. -- 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/1303#issuecomment-665638944___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Can please anybody unblock it? -- 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/1303#issuecomment-665012491___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> Looks like there's a hick-up in the CI. Re-running the semaphore tests. > > Well, trying... Thanks, apparently it's still stuck.. -- 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/1303#issuecomment-664292711___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Looks like there's a hick-up in the CI. Re-running the semaphore tests. -- 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/1303#issuecomment-664171773___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Thank you for the review. Having 2 approvals can please anybody merge it? -- 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/1303#issuecomment-664149460___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@Conan-Kudo approved this pull request. -- 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/1303#pullrequestreview-455363907___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. 11358ce35d0c98d716d68bb4824e86c44f78491b Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/0281c07a8ba82cb73abdb5488a1d8a784db91f8e..11358ce35d0c98d716d68bb4824e86c44f78491b ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@Conan-Kudo commented on this pull request. > @@ -350,7 +350,7 @@ package or when debugging this package.\ # "w9.gzdio" gzip level 9 (default). # "w9.bzdio" bzip2 level 9. # "w6.xzdio" xz level 6, xz's default. -# "w7T16.xzdio" xz level 7 using 16 thread (xz only) +# "w7T16.xzdio" xz level 7 using 16 thread (xz and zstd only) `zstd` would use `w7T16.zstdio`, so it needs its own line. -- 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/1303#discussion_r460520945___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin commented on this pull request. > @@ -350,7 +350,7 @@ package or when debugging this package.\ # "w9.gzdio" gzip level 9 (default). # "w9.bzdio" bzip2 level 9. # "w6.xzdio" xz level 6, xz's default. -# "w7T16.xzdio" xz level 7 using 16 thread (xz only) +# "w7T16.xzdio" xz level 7 using 16 thread (xz and zstd only) I understand it that the description in parenthesis is related to the threading mode. And `zstd` newly support it. Or am I wrong? -- 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/1303#discussion_r460485286___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@Conan-Kudo requested changes on this pull request. > @@ -350,7 +350,7 @@ package or when debugging this package.\ # "w9.gzdio" gzip level 9 (default). # "w9.bzdio" bzip2 level 9. # "w6.xzdio" xz level 6, xz's default. -# "w7T16.xzdio" xz level 7 using 16 thread (xz only) +# "w7T16.xzdio" xz level 7 using 16 thread (xz and zstd only) This description is wrong. You need a new line describing this for zstd. -- 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/1303#pullrequestreview-455327936___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> I'll implement support in deltarpm and you can port it to drpm, ok? Sure, appreciate that! -- 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/1303#issuecomment-663451176___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
I'll implement support in deltarpm and you can port it to drpm, ok? -- 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/1303#issuecomment-663449659___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> Looks good to me, thanks. (Of course the deltarpm and drpm need to be updated > now so that they support a ZSTD_THREADED compression type as well.) Thanks. I can start working on the support for the mentioned tools. Can you please guide me a bit what would be needed? -- 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/1303#issuecomment-663448525___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Looks good to me, thanks. (Of course the deltarpm and drpm need to be updated now so that they support a ZSTD_THREADED compression type as well.) -- 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/1303#issuecomment-663438048___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> I've just asked about this explicitly here: > [facebook/zstd#2238 > (comment)](https://github.com/facebook/zstd/issues/2238#issuecomment-662906821) And `zstd` folks confirmed that a number of threads does not influence the stability of the "threaded" compression mode. -- 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/1303#issuecomment-663370519___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> From my POV it is good to go and we can follow up with improvements like > auto-detection of cores. Thank you. Yes, I agree with that. I can imagine doing what `zstd` or `xz` do for auto-detection of cores: `xz -T0 `. -- 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/1303#issuecomment-663160218___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain approved this pull request. >From my POV it is good to go and we can follow up with improvements like >auto-detection of cores. -- 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/1303#pullrequestreview-454383007___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> I'm also not so sure about the "the multi-threaded output produces the same > compressed data no matter how many threads you use" statement, because the > block size seems to depend on the number of workers (see > ZSTDMT_compress_advanced_internal). I've just asked about this explicitly here: https://github.com/facebook/zstd/issues/2238#issuecomment-662906821 -- 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/1303#issuecomment-662907291___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
All right, I changed the logic so that threaded mode is used only when a user set a `T` value in the compression algorithm. And I've just removed the automatic deduction of number of cores for now. @mlschroe Hope it's useful 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/1303#issuecomment-662906324___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. 0281c07a8ba82cb73abdb5488a1d8a784db91f8e Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/14af110e70cc1f63e462eb11dab1230028fb8ce7..0281c07a8ba82cb73abdb5488a1d8a784db91f8e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > @@ -1072,6 +1072,7 @@ static rpmzstd rpmzstdNew(int fdno, const char *fmode) char *t = stdio; char *te = t + sizeof(stdio) - 2; int c; +int threads = -1; ```suggestion int threads = 0; ``` to satisfy @ffesti -- 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/1303#pullrequestreview-452511259___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
We need to make sure that the result with the current settings (non threaded compression) remains unchanged to not break deltarpm that way. -- 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/1303#issuecomment-661689585___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
$ rpm2cpio kdelibs3-3.5.5-39.i586.rpm | zstd -c --single-thread | wc -c 16307922 $ rpm2cpio kdelibs3-3.5.5-39.i586.rpm | zstd -c -T1 | wc -c 16308523 -- 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/1303#issuecomment-658039886___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Hmm, I don't think ZSTDMT_compress_advanced_internal is used in out use case, so disregard the comment about the influence of the thread number. (I'm not 100% sure, though, the zstd source code is somewhat cryptic to me.) -- 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/1303#issuecomment-658036765___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Also note that threading mode with one thread (T1) is *not* unthreaded mode. -- 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/1303#issuecomment-658032466___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
But was it identical? Zstd upstream said that it is different. -- 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/1303#issuecomment-658032043___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> it compresses a bit worse I was testing it on xonotic-data and it was 873M in single-threaded compression and the same size in multi-threaded mode. -- 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/1303#issuecomment-658028859___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
I'm also not so sure about the "the multi-threaded output produces the same compressed data no matter how many threads you use" statement, because the block size seems to depend on the number of workers (see ZSTDMT_compress_advanced_internal). -- 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/1303#issuecomment-658026374___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
Note that the zstd people *did* say that threaded compression has a different output than unthreaded operation: it compresses a bit worse. So the pull request will break delta rpms. Can you please change the code so that it uses unthreaded mode if there is no 'T' in the compression flags? I.e. initialize `threads` with 0 instead of -1? -- 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/1303#issuecomment-658021442___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. 14af110e70cc1f63e462eb11dab1230028fb8ce7 Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/d2465252ae96f05b874d453e7a245c2a47a4bd50..14af110e70cc1f63e462eb11dab1230028fb8ce7 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain approved this pull request. LGTM with very small suggestion. -- 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/1303#pullrequestreview-446818885___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request.
> goto err;
}
+
+ if (threads == -1)
+ threads = rpmExpandNumeric("%{getncpus}");
+ if (threads > 1)
After all I think
```suggestion
if (threads > 0)
```
should be used.
--
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/1303#pullrequestreview-446818865___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> With my suggestions, and a proper build of libzstd in Fedora it works as > expected, it fully utilizes my system. Great to hear! -- 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/1303#issuecomment-657103190___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin commented on this pull request. > goto err; } + + if (threads > 0) Good point! I changed that to `> 1`, same what lzma does. -- 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/1303#discussion_r453218566___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin commented on this pull request. > + if (c >= (int)'0' && c <= (int)'9') + threads = strtol(s-1, (char **), 10); Well, it's just a small piece of code. I would not put it to a separate function. -- 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/1303#discussion_r453218506___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. d2465252ae96f05b874d453e7a245c2a47a4bd50 Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/749bf9ecbadbbde5076bf722e1b9883e4df345ba..d2465252ae96f05b874d453e7a245c2a47a4bd50 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
With my suggestions, and a proper build of libzstd in Fedora it works as expected, it fully utilizes my system. Testing on `xonotic-data` which is 873M. ``` before: 284.05user 3.03system 4:48.64elapsed 99%CPU (0avgtext+0avgdata 153540maxresident)k 88inputs+5456312outputs (1major+80373minor)pagefaults 0swaps after: 578.54user 5.56system 1:36.86elapsed 602%CPU (0avgtext+0avgdata 1409788maxresident)k 104inputs+5455928outputs (1major+386345minor)pagefaults 0swaps ``` -- 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/1303#issuecomment-657102812___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request.
> goto err;
}
+
+ if (threads > 0)
```suggestion
if (threads == -1)
threads = rpmExpandNumeric("%{getncpus}");
if (threads > 0)
```
--
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/1303#pullrequestreview-446817519___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > goto err; } + + if (threads > 0) + if (ZSTD_isError (ZSTD_CCtx_setParameter(_stream, ZSTD_c_nbWorkers, threads))) Oh my, Fedora ships libzstd without MT support. ignore this one. -- 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/1303#discussion_r453216291___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > goto err; } + + if (threads > 0) + if (ZSTD_isError (ZSTD_CCtx_setParameter(_stream, ZSTD_c_nbWorkers, threads))) hmmpf, this always gives me an error on Fedora Rawhide: ``` warning: zstd library does not support multi-threading ``` With `ZSTD_getErrorName()` I get `Unsupported parameter`. -- 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/1303#pullrequestreview-446815348___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > goto err; } + + if (threads > 0) + if (ZSTD_isError (ZSTD_CCtx_setParameter(_stream, ZSTD_c_nbWorkers, threads))) + rpmlog(RPMLOG_WARNING, "zstd library does not support multi-threading"); ```suggestion rpmlog(RPMLOG_WARNING, "zstd library does not support multi-threading\n"); ``` -- 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/1303#pullrequestreview-446814815___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > + ++s; + c = *s; ```suggestion c = *s++; ``` -- 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/1303#pullrequestreview-446814593___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin I've tried this on my laptop and it does not seem to work? ``` $ /usr/bin/time ~/Projects/upstream/rpm/rpmbuild -bb xonotic-data.spec -D "_sourcedir $PWD" -D "_binary_payload w19T8.zstdio" ... 289.90user 3.08system 4:54.67elapsed 99%CPU (0avgtext+0avgdata 155348maxresident)k 0inputs+5456184outputs (0major+72889minor)pagefaults 0swaps ``` -- 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/1303#issuecomment-657091568___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. > goto err; } + + if (threads > 0) hmmm, thinking about it more.. ``` /* These parameters are only useful if multi-threading is enabled (compiled with build macro ZSTD_MULTITHREAD). ``` And checking code, yeah - it will return and error. so should we instead change it to `> 1`? -- 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/1303#discussion_r453209459___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request.
LGTM with small nitpicks
> goto err;
}
+
+ if (threads > 0)
If I read https://facebook.github.io/zstd/zstd_manual.html correctly, even if
you set it to `0`, it will use single thread which makes this if unneeded.
> @@ -1100,7 +1101,12 @@ static rpmzstd rpmzstdNew(int fdno, const char *fmode)
flags &= ~O_ACCMODE;
flags |= O_RDWR;
continue;
- break;
+ case 'T':
I think it would be nice to support setting it to -1, same as for lzma:
```
if (threads == -1)
threads = rpmExpandNumeric("%{getncpus}");
```
> + if (c >= (int)'0' && c <= (int)'9')
+ threads = strtol(s-1, (char **), 10);
Just curious if it would make sense to change lzopen_internal() to follow the
same logic? If so, could you push a separate commit for that?
--
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/1303#pullrequestreview-446799809___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin commented on this pull request. >AS_IF([test "$enable_zstd" = "yes"], [ if test "$have_zstd" = "no"; then AC_MSG_ERROR([--enable-zstd specified, but not available]) fi ]) + PKG_CHECK_MODULES([ZSTD], [libzstd], [have_zstd=yes], [have_zstd=no]) Oh yeah, it is new to me! Thanks. -- 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/1303#discussion_r453176326___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 2 commits. 077e20cb996777e8b3e918c4e520005c73df37bc zstd compression: port to the new API. 749bf9ecbadbbde5076bf722e1b9883e4df345ba Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/2d42b78f50fb7b4f3848f0ceae916595214a5b89..749bf9ecbadbbde5076bf722e1b9883e4df345ba ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@ignatenkobrain commented on this pull request. >AS_IF([test "$enable_zstd" = "yes"], [ if test "$have_zstd" = "no"; then AC_MSG_ERROR([--enable-zstd specified, but not available]) fi ]) + PKG_CHECK_MODULES([ZSTD], [libzstd], [have_zstd=yes], [have_zstd=no]) I think it is much easier to have here `libzstd >= 1.3.8` rather than custom C preproc check. -- 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/1303#pullrequestreview-446554479___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
> This change looks good and straight forward. Thanks! > > I am missing some information on what version of zstd supports the new API - > and if necessary a check in configure.ac. Updated `configure.ac` (one needs at least release `1.3.8`). -- 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/1303#issuecomment-656696346___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
This change looks good and straight forward. I am missing some information on what version of zstd supports the new API - and if necessary a check in configure.ac. -- 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/1303#issuecomment-656683723___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)
@marxin pushed 1 commit. 1d965759205bf5f072724c397004ba1a433f7c7f Support threading for zstd compression. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1303/files/fff307dd41294192560a33cca09e3675a0af925a..1d965759205bf5f072724c397004ba1a433f7c7f ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
