From ee84817fe7742c7d896250972d260e61a02ab144 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Wed, 14 Oct 2020 22:06:08 -0400 Subject: [PATCH 01/10] Reset posInSequence when using ZSTD_referenceExternalSequences() --- lib/compress/zstd_compress.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index b1bb9fa10..2f1a3f3ab 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2853,6 +2853,7 @@ size_t ZSTD_referenceExternalSequences(ZSTD_CCtx* cctx, rawSeq* seq, size_t nbSe cctx->externSeqStore.size = nbSeq; cctx->externSeqStore.capacity = nbSeq; cctx->externSeqStore.pos = 0; + cctx->externSeqStore.posInSequence = 0; return 0; } From 4d01979b6255e0ccc2b8ba868c9e769bf00b60c2 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Fri, 16 Oct 2020 20:30:00 -0400 Subject: [PATCH 02/10] Expose and call ZSTD_ldm_skipRawSeqStoreBytes() --- lib/compress/zstd_compress.c | 6 +++++- lib/compress/zstd_ldm.c | 5 +---- lib/compress/zstd_ldm.h | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 2f1a3f3ab..de2426fce 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2374,7 +2374,11 @@ static size_t ZSTD_buildSeqStore(ZSTD_CCtx* zc, const void* src, size_t srcSize) /* Assert that we have correctly flushed the ctx params into the ms's copy */ ZSTD_assertEqualCParams(zc->appliedParams.cParams, ms->cParams); if (srcSize < MIN_CBLOCK_SIZE+ZSTD_blockHeaderSize+1) { - ZSTD_ldm_skipSequences(&zc->externSeqStore, srcSize, zc->appliedParams.cParams.minMatch); + if (zc->appliedParams.cParams.strategy >= ZSTD_btopt) { + ZSTD_ldm_skipRawSeqStoreBytes(&zc->externSeqStore, srcSize); + } else { + ZSTD_ldm_skipSequences(&zc->externSeqStore, srcSize, zc->appliedParams.cParams.minMatch); + } return ZSTDbss_noCompress; /* don't even attempt compression below a certain srcSize */ } ZSTD_resetSeqStore(&(zc->seqStore)); diff --git a/lib/compress/zstd_ldm.c b/lib/compress/zstd_ldm.c index cc338e697..dfbd40695 100644 --- a/lib/compress/zstd_ldm.c +++ b/lib/compress/zstd_ldm.c @@ -555,10 +555,7 @@ static rawSeq maybeSplitSequence(rawSeqStore_t* rawSeqStore, return sequence; } -/* ZSTD_ldm_skipRawSeqStoreBytes(): - * Moves forward in rawSeqStore by nbBytes, updating fields 'pos' and 'posInSequence'. - */ -static void ZSTD_ldm_skipRawSeqStoreBytes(rawSeqStore_t* rawSeqStore, size_t nbBytes) { +void ZSTD_ldm_skipRawSeqStoreBytes(rawSeqStore_t* rawSeqStore, size_t nbBytes) { U32 currPos = (U32)(rawSeqStore->posInSequence + nbBytes); while (currPos && rawSeqStore->pos < rawSeqStore->size) { rawSeq currSeq = rawSeqStore->seq[rawSeqStore->pos]; diff --git a/lib/compress/zstd_ldm.h b/lib/compress/zstd_ldm.h index 229ea05a9..6561024e4 100644 --- a/lib/compress/zstd_ldm.h +++ b/lib/compress/zstd_ldm.h @@ -78,6 +78,12 @@ size_t ZSTD_ldm_blockCompress(rawSeqStore_t* rawSeqStore, void ZSTD_ldm_skipSequences(rawSeqStore_t* rawSeqStore, size_t srcSize, U32 const minMatch); +/* ZSTD_ldm_skipRawSeqStoreBytes(): + * Moves forward in rawSeqStore by nbBytes, updating fields 'pos' and 'posInSequence'. + * Not to be used in conjunction with ZSTD_ldm_skipSequences(). + * Must be called for data with is not passed to ZSTD_ldm_blockCompress(). + */ +void ZSTD_ldm_skipRawSeqStoreBytes(rawSeqStore_t* rawSeqStore, size_t nbBytes); /** ZSTD_ldm_getTableSize() : * Estimate the space needed for long distance matching tables or 0 if LDM is From 590f7f55f0ca68bb1a90043efb014f18c4b07cd1 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Mon, 19 Oct 2020 10:26:17 -0400 Subject: [PATCH 03/10] Add ldm enable condition in ZSTD_resetCCtx_internal --- lib/compress/zstd_compress.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index b1bb9fa10..434c23000 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1542,6 +1542,12 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, zc->isFirstBlock = 1; + if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { + /* Enable LDM by default for optimal parser and window size >= 128MB */ + DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); + params.ldmParams.enableLdm = 1; + } + if (params.ldmParams.enableLdm) { /* Adjust long distance matching parameters */ ZSTD_ldm_adjustParameters(¶ms.ldmParams, ¶ms.cParams); From df470e176bdd4240a18997f4cf57e100c0ac6755 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Mon, 19 Oct 2020 10:52:41 -0400 Subject: [PATCH 04/10] Add unit test for no cctx requested params change --- tests/fuzzer.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 302bfaf62..8c310f3d4 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -1196,6 +1196,26 @@ static int basicUnitTests(U32 const seed, double compressibility) } DISPLAYLEVEL(3, "OK \n"); + DISPLAYLEVEL(3, "test%3d : ldm conditionally enabled by default doesn't change cctx params: ", testNb++); + { ZSTD_CCtx* const cctx = ZSTD_createCCtx(); + ZSTD_outBuffer out = {NULL, 0, 0}; + ZSTD_inBuffer in = {NULL, 0, 0}; + int value; + + /* Even if LDM will be enabled by default in the applied params (since wlog >= 27 and strategy >= btopt), + * we should not modify the actual parameter specified by the user within the CCtx + */ + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_windowLog, 27)); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_strategy, ZSTD_btopt)); + + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_continue)); + CHECK_Z(ZSTD_CCtx_getParameter(cctx, ZSTD_c_enableLongDistanceMatching, &value)); + CHECK_EQ(value, 0); + + ZSTD_freeCCtx(cctx); + } + DISPLAYLEVEL(3, "OK \n"); + /* this test is really too long, and should be made faster */ DISPLAYLEVEL(3, "test%3d : overflow protection with large windowLog : ", testNb++); { ZSTD_CCtx* const cctx = ZSTD_createCCtx(); From aad436da37168bfa4860db92c2669905426cefc3 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Mon, 19 Oct 2020 11:02:29 -0400 Subject: [PATCH 05/10] Document ldm enabled by default in zstd.h --- lib/zstd.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/zstd.h b/lib/zstd.h index 75d5bb237..733595d9c 100644 --- a/lib/zstd.h +++ b/lib/zstd.h @@ -339,7 +339,9 @@ typedef enum { * for large inputs, by finding large matches at long distance. * It increases memory usage and window size. * Note: enabling this parameter increases default ZSTD_c_windowLog to 128 MB - * except when expressly set to a different value. */ + * except when expressly set to a different value. + * Note: will be enabled by default if ZSTD_c_windowLog >= 128 MB and + * compression strategy >= ZSTD_btopt (== compression level 16+) */ ZSTD_c_ldmHashLog=161, /* Size of the table for long distance matching, as a power of 2. * Larger values increase memory usage and compression ratio, * but decrease compression speed. From b1c7fc57687e208b82bcbe46387f6de5d5052d94 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Mon, 19 Oct 2020 12:07:06 -0400 Subject: [PATCH 06/10] Add compatibility for multithreading --- lib/compress/zstd_compress.c | 2 +- lib/compress/zstdmt_compress.c | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 434c23000..0bfaeec4f 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1542,7 +1542,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, zc->isFirstBlock = 1; - if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { + if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27 && params.nbWorkers == 0) { /* Enable LDM by default for optimal parser and window size >= 128MB */ DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); params.ldmParams.enableLdm = 1; diff --git a/lib/compress/zstdmt_compress.c b/lib/compress/zstdmt_compress.c index d68f47189..bf4782cdd 100644 --- a/lib/compress/zstdmt_compress.c +++ b/lib/compress/zstdmt_compress.c @@ -1459,6 +1459,13 @@ size_t ZSTDMT_initCStream_internal( mtctx->params = params; mtctx->frameContentSize = pledgedSrcSize; + + if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { + /* Enable LDM by default for optimal parser and window size >= 128MB */ + DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); + params.ldmParams.enableLdm = 1; + } + if (dict) { ZSTD_freeCDict(mtctx->cdictLocal); mtctx->cdictLocal = ZSTD_createCDict_advanced(dict, dictSize, From d28d8a1d725ca1885a69f012dd0525d59c5b066e Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Mon, 19 Oct 2020 15:22:10 -0400 Subject: [PATCH 07/10] Include LDM tables size for CCtx size estimation where relevant --- lib/compress/zstd_compress.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 0bfaeec4f..5e7fd5374 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -209,6 +209,16 @@ static ZSTD_CCtx_params ZSTD_makeCCtxParamsFromCParams( /* should not matter, as all cParams are presumed properly defined */ ZSTD_CCtxParams_init(&cctxParams, ZSTD_CLEVEL_DEFAULT); cctxParams.cParams = cParams; + + if (cParams.strategy >= ZSTD_btopt && cParams.windowLog >= 27 && cctxParams.nbWorkers == 0) { + DEBUGLOG(4, "ZSTD_makeCCtxParamsFromCParams(): Including LDM into cctx params"); + cctxParams.ldmParams.enableLdm = 1; + /* LDM is enabled by default for optimal parser and window size >= 128MB */ + ZSTD_ldm_adjustParameters(&cctxParams.ldmParams, &cParams); + assert(cctxParams.ldmParams.hashLog >= cctxParams.ldmParams.bucketSizeLog); + assert(cctxParams.ldmParams.hashRateLog < 32); + } + assert(!ZSTD_checkCParams(cParams)); return cctxParams; } From 578e889ec1797b96f62cf73480d0ba3b3823b79c Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Tue, 20 Oct 2020 13:01:04 -0400 Subject: [PATCH 08/10] Move ldm enable to compressStream2() --- lib/compress/zstd_compress.c | 11 +++++------ lib/compress/zstdmt_compress.c | 7 ------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 5e7fd5374..6d5da4b31 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1552,12 +1552,6 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, zc->isFirstBlock = 1; - if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27 && params.nbWorkers == 0) { - /* Enable LDM by default for optimal parser and window size >= 128MB */ - DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); - params.ldmParams.enableLdm = 1; - } - if (params.ldmParams.enableLdm) { /* Adjust long distance matching parameters */ ZSTD_ldm_adjustParameters(¶ms.ldmParams, ¶ms.cParams); @@ -4194,6 +4188,11 @@ size_t ZSTD_compressStream2( ZSTD_CCtx* cctx, dictSize, mode); } + if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { + /* Enable LDM by default for optimal parser and window size >= 128MB */ + DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); + params.ldmParams.enableLdm = 1; + } #ifdef ZSTD_MULTITHREAD if ((cctx->pledgedSrcSizePlusOne-1) <= ZSTDMT_JOBSIZE_MIN) { diff --git a/lib/compress/zstdmt_compress.c b/lib/compress/zstdmt_compress.c index bf4782cdd..d68f47189 100644 --- a/lib/compress/zstdmt_compress.c +++ b/lib/compress/zstdmt_compress.c @@ -1459,13 +1459,6 @@ size_t ZSTDMT_initCStream_internal( mtctx->params = params; mtctx->frameContentSize = pledgedSrcSize; - - if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { - /* Enable LDM by default for optimal parser and window size >= 128MB */ - DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); - params.ldmParams.enableLdm = 1; - } - if (dict) { ZSTD_freeCDict(mtctx->cdictLocal); mtctx->cdictLocal = ZSTD_createCDict_advanced(dict, dictSize, From 8bdb32aebe181ebfa62237be49e9d183c2daf596 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Tue, 20 Oct 2020 13:46:02 -0400 Subject: [PATCH 09/10] Add a function for LDM enable check --- lib/compress/zstd_compress.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 6d5da4b31..404f38dac 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -202,6 +202,14 @@ size_t ZSTD_sizeof_CStream(const ZSTD_CStream* zcs) /* private API call, for dictBuilder only */ const seqStore_t* ZSTD_getSeqStore(const ZSTD_CCtx* ctx) { return &(ctx->seqStore); } +/* Returns 1 if compression parameters are such that we should + * enable long distance matching (wlog >= 27, strategy >= btopt). + * Returns 0 otherwise. + */ +static U32 ZSTD_CParams_shouldEnableLdm(const ZSTD_compressionParameters* const cParams) { + return cParams->strategy >= ZSTD_btopt && cParams->windowLog >= 27; +} + static ZSTD_CCtx_params ZSTD_makeCCtxParamsFromCParams( ZSTD_compressionParameters cParams) { @@ -210,7 +218,7 @@ static ZSTD_CCtx_params ZSTD_makeCCtxParamsFromCParams( ZSTD_CCtxParams_init(&cctxParams, ZSTD_CLEVEL_DEFAULT); cctxParams.cParams = cParams; - if (cParams.strategy >= ZSTD_btopt && cParams.windowLog >= 27 && cctxParams.nbWorkers == 0) { + if (ZSTD_CParams_shouldEnableLdm(&cParams)) { DEBUGLOG(4, "ZSTD_makeCCtxParamsFromCParams(): Including LDM into cctx params"); cctxParams.ldmParams.enableLdm = 1; /* LDM is enabled by default for optimal parser and window size >= 128MB */ @@ -4188,7 +4196,7 @@ size_t ZSTD_compressStream2( ZSTD_CCtx* cctx, dictSize, mode); } - if (params.cParams.strategy >= ZSTD_btopt && params.cParams.windowLog >= 27) { + if (ZSTD_CParams_shouldEnableLdm(¶ms.cParams)) { /* Enable LDM by default for optimal parser and window size >= 128MB */ DEBUGLOG(4, "LDM enabled by default (window size >= 128MB, strategy >= btopt)"); params.ldmParams.enableLdm = 1; From 1f661b5f6b9c84fa4ddd2128e05a7ce0e3db6d9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Ketelaars?= Date: Tue, 27 Oct 2020 16:51:24 +0100 Subject: [PATCH 10/10] 'head -c BYTES' is non-portable --- lib/Makefile | 2 +- programs/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Makefile b/lib/Makefile index b8295ff03..b66f81608 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -185,7 +185,7 @@ HASH ?= md5sum HAVE_HASH := $(shell echo 1 | $(HASH) > /dev/null && echo 1 || echo 0) ifeq ($(HAVE_HASH), 1) - HASH_VALUE := $(shell echo $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(ZSTD_FILES) | $(HASH) | head -c 16) + HASH_VALUE := $(shell echo $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(ZSTD_FILES) | $(HASH) | dd bs=1 count=16 status=none) HASH_DIR := conf_$(HASH_VALUE) else $(info warning : could not find hash function to differentiate builds with different flags) diff --git a/programs/Makefile b/programs/Makefile index 70fb2c32d..8f27f1539 100644 --- a/programs/Makefile +++ b/programs/Makefile @@ -107,7 +107,7 @@ HASH ?= md5sum HAVE_HASH := $(shell echo 1 | $(HASH) > /dev/null && echo 1 || echo 0) ifeq ($(HAVE_HASH), 1) - HASH_VALUE := $(shell echo $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) | $(HASH) | head -c 16) + HASH_VALUE := $(shell echo $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) | $(HASH) | dd bs=1 count=16 status=none) HASH_DIR := conf_$(HASH_VALUE) else $(info warning : could not find hash function to differentiate builds with different flags)