From 4c202815c71a18d47a83f1f0109fe1d9b77debb6 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 12:40:59 -0700 Subject: [PATCH 1/8] [pzstd] Smart default # of threads (#331) --- contrib/pzstd/Options.cpp | 11 +++++++++-- contrib/pzstd/test/OptionsTest.cpp | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/contrib/pzstd/Options.cpp b/contrib/pzstd/Options.cpp index 616130996..122f4fb36 100644 --- a/contrib/pzstd/Options.cpp +++ b/contrib/pzstd/Options.cpp @@ -10,6 +10,7 @@ #include #include +#include namespace pzstd { @@ -103,6 +104,7 @@ bool Options::parse(int argc, const char** argv) { numThreads = parseUnsigned(argv[i]); if (numThreads == 0) { std::fprintf(stderr, "Invalid argument: # of threads must be > 0.\n"); + return false; } break; case 'p': @@ -169,12 +171,17 @@ bool Options::parse(int argc, const char** argv) { if (compressionLevel > maxCLevel) { std::fprintf( stderr, "Invalid compression level %u.\n", compressionLevel); + return false; } } // Check that numThreads is set if (numThreads == 0) { - std::fprintf(stderr, "Invalid arguments: # of threads not specified.\n"); - return false; + numThreads = std::thread::hardware_concurrency(); + if (numThreads == 0) { + std::fprintf(stderr, "Invalid arguments: # of threads not specified " + "and unable to determine hardware concurrency.\n"); + return false; + } } return true; } diff --git a/contrib/pzstd/test/OptionsTest.cpp b/contrib/pzstd/test/OptionsTest.cpp index 87e79d59e..b87358c04 100644 --- a/contrib/pzstd/test/OptionsTest.cpp +++ b/contrib/pzstd/test/OptionsTest.cpp @@ -118,11 +118,11 @@ TEST(Options, ValidInputs) { } } -TEST(Options, BadNumThreads) { +TEST(Options, NumThreads) { { Options options; std::array args = {{nullptr, "-o", "-"}}; - EXPECT_FALSE(options.parse(args.size(), args.data())); + EXPECT_TRUE(options.parse(args.size(), args.data())); } { Options options; From 378d12bb0c261fdfe97e5d3d4fcd271117601391 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 12:43:07 -0700 Subject: [PATCH 2/8] [pzstd] Changes to compile on VS2015 --- contrib/pzstd/Pzstd.cpp | 6 +++--- contrib/pzstd/utils/FileSystem.h | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/contrib/pzstd/Pzstd.cpp b/contrib/pzstd/Pzstd.cpp index ddfa59556..09eab90f1 100644 --- a/contrib/pzstd/Pzstd.cpp +++ b/contrib/pzstd/Pzstd.cpp @@ -41,7 +41,7 @@ size_t pzstdMain(const Options& options, ErrorHolder& errorHolder) { return 0; } std::error_code ec; - inputSize = file_size(options.inputFile, ec); + inputSize = static_cast(file_size(options.inputFile, ec)); if (ec) { inputSize = 0; } @@ -155,7 +155,7 @@ static void compress( ZSTD_parameters parameters) { auto guard = makeScopeGuard([&] { out->finish(); }); // Initialize the CCtx - std::unique_ptr ctx( + std::unique_ptr ctx( ZSTD_createCStream(), ZSTD_freeCStream); if (!errorHolder.check(ctx != nullptr, "Failed to allocate ZSTD_CStream")) { return; @@ -311,7 +311,7 @@ static void decompress( std::shared_ptr out) { auto guard = makeScopeGuard([&] { out->finish(); }); // Initialize the DCtx - std::unique_ptr ctx( + std::unique_ptr ctx( ZSTD_createDStream(), ZSTD_freeDStream); if (!errorHolder.check(ctx != nullptr, "Failed to allocate ZSTD_DStream")) { return; diff --git a/contrib/pzstd/utils/FileSystem.h b/contrib/pzstd/utils/FileSystem.h index deae0b5b7..cb682819d 100644 --- a/contrib/pzstd/utils/FileSystem.h +++ b/contrib/pzstd/utils/FileSystem.h @@ -20,7 +20,7 @@ namespace pzstd { -using file_status = struct stat; +using file_status = struct ::stat; /// http://en.cppreference.com/w/cpp/filesystem/status inline file_status status(StringPiece path, std::error_code& ec) noexcept { @@ -35,7 +35,13 @@ inline file_status status(StringPiece path, std::error_code& ec) noexcept { /// http://en.cppreference.com/w/cpp/filesystem/is_regular_file inline bool is_regular_file(file_status status) noexcept { +#if defined(S_ISREG) return S_ISREG(status.st_mode); +#elif !defined(S_ISREG) && defined(S_IFMT) && defined(S_IFREG) + return (status.st_mode & S_IFMT) == S_IFREG; +#else + static_assert(false, "No POSIX stat() support."); +#endif } /// http://en.cppreference.com/w/cpp/filesystem/is_regular_file From 4db9fbdec729726b2dab59748f1ec5c2fd47b1d6 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 14:00:20 -0700 Subject: [PATCH 3/8] [pzstd] Compile with minGW 64 --- contrib/pzstd/utils/FileSystem.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contrib/pzstd/utils/FileSystem.h b/contrib/pzstd/utils/FileSystem.h index cb682819d..979c82b7a 100644 --- a/contrib/pzstd/utils/FileSystem.h +++ b/contrib/pzstd/utils/FileSystem.h @@ -11,6 +11,7 @@ #include "utils/Range.h" #include +#include #include #include @@ -20,12 +21,21 @@ namespace pzstd { +#if defined(_MSC_VER) +using file_status = struct ::_stat64; +#else using file_status = struct ::stat; +#endif /// http://en.cppreference.com/w/cpp/filesystem/status inline file_status status(StringPiece path, std::error_code& ec) noexcept { file_status status; - if (stat(path.data(), &status)) { +#if defined(_MSC_VER) + const auto error = ::_stat64(path.data(), &status); +#else + const auto error = ::stat(path.data(), &status); +#endif + if (error) { ec.assign(errno, std::generic_category()); } else { ec.clear(); From b3ed23e18ea6c6e0a1f8b86354716e255c650850 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 14:00:55 -0700 Subject: [PATCH 4/8] [pzstd] Add appveyor build commands --- appveyor.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index 4e21db1c4..f5eab80d8 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -45,7 +45,13 @@ build_script: ECHO make %CLANG_PARAMS% && make %CLANG_PARAMS% && COPY tests\fuzzer.exe tests\fuzzer_clang.exe && - make clean + make clean && + ECHO *** && + ECHO *** Building pzstd for %PLATFORM% && + ECHO *** && + ECHO make -C contrib\pzstd pzstd && + make -C contrib\pzstd pzstd && + make -C contrib\pzstd clean ) - if [%COMPILER%]==[gcc] ( ECHO *** && From 823bf3d08de6aff1b0fd10f384bf72e727412d07 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 20:11:02 -0700 Subject: [PATCH 5/8] Fix invalid narrowing conversion to size_t --- contrib/pzstd/Pzstd.cpp | 19 +++++++++++-------- contrib/pzstd/Pzstd.h | 3 ++- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/contrib/pzstd/Pzstd.cpp b/contrib/pzstd/Pzstd.cpp index 09eab90f1..0caf0f933 100644 --- a/contrib/pzstd/Pzstd.cpp +++ b/contrib/pzstd/Pzstd.cpp @@ -34,14 +34,14 @@ using std::size_t; size_t pzstdMain(const Options& options, ErrorHolder& errorHolder) { // Open the input file and attempt to determine its size FILE* inputFd = stdin; - size_t inputSize = 0; + std::uintmax_t inputSize = 0; if (options.inputFile != "-") { inputFd = std::fopen(options.inputFile.c_str(), "rb"); if (!errorHolder.check(inputFd != nullptr, "Failed to open input file")) { return 0; } std::error_code ec; - inputSize = static_cast(file_size(options.inputFile, ec)); + inputSize = file_size(options.inputFile, ec); if (ec) { inputSize = 0; } @@ -217,14 +217,17 @@ static void compress( * @param numThreads The number of threads available to run compression jobs on * @param params The zstd parameters to be used for compression */ -static size_t -calculateStep(size_t size, size_t numThreads, const ZSTD_parameters& params) { +static size_t calculateStep( + std::uintmax_t size, + size_t numThreads, + const ZSTD_parameters ¶ms) { size_t step = 1ul << (params.cParams.windowLog + 2); // If file size is known, see if a smaller step will spread work more evenly if (size != 0) { - size_t newStep = size / numThreads; - if (newStep != 0) { - step = std::min(step, newStep); + const std::uintmax_t newStep = size / std::uintmax_t{numThreads}; + if (newStep != 0 && + newStep <= std::uintmax_t{std::numeric_limits::max()}) { + step = std::min(step, size_t{newStep}); } } return step; @@ -268,7 +271,7 @@ void asyncCompressChunks( WorkQueue>& chunks, ThreadPool& executor, FILE* fd, - size_t size, + std::uintmax_t size, size_t numThreads, ZSTD_parameters params) { auto chunksGuard = makeScopeGuard([&] { chunks.finish(); }); diff --git a/contrib/pzstd/Pzstd.h b/contrib/pzstd/Pzstd.h index 617aecb3f..51d15846c 100644 --- a/contrib/pzstd/Pzstd.h +++ b/contrib/pzstd/Pzstd.h @@ -19,6 +19,7 @@ #undef ZSTD_STATIC_LINKING_ONLY #include +#include #include namespace pzstd { @@ -52,7 +53,7 @@ void asyncCompressChunks( WorkQueue>& chunks, ThreadPool& executor, FILE* fd, - std::size_t size, + std::uintmax_t size, std::size_t numThreads, ZSTD_parameters parameters); From 4d4d1ad3b33514d0c1303c6efcceb7ede278e1c9 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 6 Sep 2016 20:27:11 -0700 Subject: [PATCH 6/8] Fix minor potential narrowing bug --- contrib/pzstd/Pzstd.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/pzstd/Pzstd.cpp b/contrib/pzstd/Pzstd.cpp index 0caf0f933..87c4c202f 100644 --- a/contrib/pzstd/Pzstd.cpp +++ b/contrib/pzstd/Pzstd.cpp @@ -221,7 +221,7 @@ static size_t calculateStep( std::uintmax_t size, size_t numThreads, const ZSTD_parameters ¶ms) { - size_t step = 1ul << (params.cParams.windowLog + 2); + size_t step = size_t{1} << (params.cParams.windowLog + 2); // If file size is known, see if a smaller step will spread work more evenly if (size != 0) { const std::uintmax_t newStep = size / std::uintmax_t{numThreads}; From 0e07bf3f605f670a27e430dc8d05508d3c459c42 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 7 Sep 2016 06:33:02 +0200 Subject: [PATCH 7/8] added comments on searchLength min / max (#337) --- lib/zstd.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/zstd.h b/lib/zstd.h index 480612cfb..e05fc2936 100644 --- a/lib/zstd.h +++ b/lib/zstd.h @@ -299,8 +299,8 @@ ZSTDLIB_API size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* outp #define ZSTD_HASHLOG3_MAX 17 #define ZSTD_SEARCHLOG_MAX (ZSTD_WINDOWLOG_MAX-1) #define ZSTD_SEARCHLOG_MIN 1 -#define ZSTD_SEARCHLENGTH_MAX 7 -#define ZSTD_SEARCHLENGTH_MIN 3 +#define ZSTD_SEARCHLENGTH_MAX 7 /* only for ZSTD_fast, other strategies are limited to 6 */ +#define ZSTD_SEARCHLENGTH_MIN 3 /* only for ZSTD_btopt, other strategies are limited to 4 */ #define ZSTD_TARGETLENGTH_MIN 4 #define ZSTD_TARGETLENGTH_MAX 999 From aad9fe54703a236e525fc02eb2370b96bbb3f6c1 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 7 Sep 2016 07:00:08 +0200 Subject: [PATCH 8/8] don't remove() /dev/null (#316) --- programs/fileio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/programs/fileio.c b/programs/fileio.c index b7b201e02..02f42e891 100644 --- a/programs/fileio.c +++ b/programs/fileio.c @@ -732,7 +732,10 @@ static int FIO_decompressDstFile(dRess_t ress, result = FIO_decompressSrcFile(ress, srcFileName); if (fclose(ress.dstFile)) EXM_THROW(38, "Write error : cannot properly close %s", dstFileName); - if (result != 0) if (remove(dstFileName)) result=1; /* don't do anything if remove fails */ + if ( (result != 0) + && strcmp(dstFileName, nulmark) /* special case : don't remove() /dev/null (#316) */ + && remove(dstFileName) ) + result=1; /* don't do anything special if remove fails */ return result; }