Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-30 Thread Florian Festi
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-30 Thread Florian Festi
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-30 Thread marxin
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-29 Thread marxin
> 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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-29 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-29 Thread Florian Festi
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-28 Thread marxin
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-27 Thread marxin
> 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-27 Thread Florian Festi
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-27 Thread marxin
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-26 Thread ニール・ゴンパ
@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___

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-26 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-26 Thread ニール・ゴンパ
@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. -#

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-26 Thread marxin
@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"

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-25 Thread ニール・ゴンパ
@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. -#

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-24 Thread marxin
> 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-24 Thread Michael Schroeder
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-24 Thread marxin
> 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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-24 Thread Michael Schroeder
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-24 Thread marxin
> 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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-23 Thread marxin
> 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.

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-23 Thread Igor Raits
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-23 Thread marxin
> 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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-23 Thread marxin
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-23 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-21 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-21 Thread Florian Festi
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
$ 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
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.

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Igor Raits
> 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-14 Thread Michael Schroeder
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.

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@ignatenkobrain 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
> 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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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");

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread Igor Raits
@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. > @@

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-11 Thread marxin
@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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-10 Thread Igor Raits
@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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-10 Thread marxin
> 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

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-10 Thread Florian Festi
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:

Re: [Rpm-maint] [rpm-software-management/rpm] Support threading for zstd compression. (#1303)

2020-07-10 Thread marxin
@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: