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:
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
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:
> 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
@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:
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
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
> 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:
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:
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:
@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___
@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:
@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.
-#
@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"
@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.
-#
> 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:
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:
> 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
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:
> 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
> 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.
@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:
> 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
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
@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:
@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
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:
$ 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:
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.
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:
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:
> 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:
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
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.
@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:
@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:
@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
> 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:
@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:
@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
@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:
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
@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
@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
@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
@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");
@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:
@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
@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
@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.
> @@
@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
@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:
@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
> 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
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:
@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:
56 matches
Mail list logo
