@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:
18 matches
Mail list logo
