From 998a93b784798a6cab9d4a536a207f863ea3bcf3 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 29 Nov 2017 16:13:05 -0800 Subject: [PATCH 1/5] simplified ZSTD_CCtx_setParametersUsingCCtxParams() Any ZSTD_CCtx_setParameter() shall just write the requested parameter, without further action. Any action shall be taken at parameter application only (during init). It makes it possible to just copy CCtxParams from external container to internal state, and get rid of the more complex code which was trying to compensate for missing actions. --- lib/compress/zstd_compress.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 858eaca30..e6e0c0210 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -271,11 +271,11 @@ size_t ZSTD_CCtx_setParameter(ZSTD_CCtx* cctx, ZSTD_cParameter param, unsigned v case ZSTD_p_forceMaxWindow : /* Force back-references to remain < windowSize, * even when referencing into Dictionary content * default : 0 when using a CDict, 1 when using a Prefix */ - cctx->loadedDictEnd = 0; + cctx->loadedDictEnd = 0; /* ? */ return ZSTD_CCtxParam_setParameter(&cctx->requestedParams, param, value); case ZSTD_p_nbThreads: - if (value > 1 && cctx->staticSize) { + if ((value > 1) && cctx->staticSize) { return ERROR(parameter_unsupported); /* MT not compatible with static alloc */ } return ZSTD_CCtxParam_setParameter(&cctx->requestedParams, param, value); @@ -465,14 +465,17 @@ size_t ZSTD_CCtxParam_setParameter( * The multithreading parameters jobSize and overlapSizeLog are set only if * nbThreads > 1. * - * Pledged srcSize is treated as unknown. + * pledgedSrcSize is considered unknown */ size_t ZSTD_CCtx_setParametersUsingCCtxParams( - ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params) + ZSTD_CCtx* const cctx, const ZSTD_CCtx_params* const params) { if (cctx->streamStage != zcss_init) return ERROR(stage_wrong); if (cctx->cdict) return ERROR(stage_wrong); +#if 1 + cctx->requestedParams = *params; +#else /* Assume the compression and frame parameters are validated */ cctx->requestedParams.cParams = params->cParams; cctx->requestedParams.fParams = params->fParams; @@ -492,6 +495,7 @@ size_t ZSTD_CCtx_setParametersUsingCCtxParams( /* Copy long distance matching parameters */ cctx->requestedParams.ldmParams = params->ldmParams; +#endif /* customMem is used only for create/free params and can be ignored */ return 0; @@ -2858,6 +2862,7 @@ size_t ZSTD_compress_generic (ZSTD_CCtx* cctx, if (cctx->mtctx == NULL) return ERROR(memory_allocation); } DEBUGLOG(4, "call ZSTDMT_initCStream_internal as nbThreads=%u", params.nbThreads); + DEBUGLOG(2, "params.windowLog = %u", params.cParams.windowLog); CHECK_F( ZSTDMT_initCStream_internal( cctx->mtctx, prefixDict.dict, prefixDict.dictSize, ZSTD_dm_rawContent, From d3c59edac9e0bf544b7414e5442448f7a67b55ff Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 29 Nov 2017 16:42:20 -0800 Subject: [PATCH 2/5] removed long-range-mode tests from `zstreamtest --no-big-tests` --- lib/compress/zstd_compress.c | 3 ++- lib/compress/zstdmt_compress.c | 2 +- tests/zstreamtest.c | 33 +++++++++++++++++++-------------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index e6e0c0210..dd12b171b 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -241,6 +241,7 @@ static ZSTD_CCtx_params ZSTD_assignParamsToCCtxParams( size_t ZSTD_CCtx_setParameter(ZSTD_CCtx* cctx, ZSTD_cParameter param, unsigned value) { + DEBUGLOG(4, "ZSTD_CCtx_setParameter"); if (cctx->streamStage != zcss_init) return ERROR(stage_wrong); switch(param) @@ -320,6 +321,7 @@ size_t ZSTD_CCtxParam_setParameter( return CCtxParams->compressionLevel; case ZSTD_p_windowLog : + DEBUGLOG(4, "setting windowLog=%u", value); if (value) { /* 0 : does not change current windowLog */ CLAMPCHECK(value, ZSTD_WINDOWLOG_MIN, ZSTD_WINDOWLOG_MAX); ZSTD_cLevelToCCtxParams(CCtxParams); @@ -2862,7 +2864,6 @@ size_t ZSTD_compress_generic (ZSTD_CCtx* cctx, if (cctx->mtctx == NULL) return ERROR(memory_allocation); } DEBUGLOG(4, "call ZSTDMT_initCStream_internal as nbThreads=%u", params.nbThreads); - DEBUGLOG(2, "params.windowLog = %u", params.cParams.windowLog); CHECK_F( ZSTDMT_initCStream_internal( cctx->mtctx, prefixDict.dict, prefixDict.dictSize, ZSTD_dm_rawContent, diff --git a/lib/compress/zstdmt_compress.c b/lib/compress/zstdmt_compress.c index 2926d3b48..ce813dccc 100644 --- a/lib/compress/zstdmt_compress.c +++ b/lib/compress/zstdmt_compress.c @@ -140,7 +140,7 @@ static size_t ZSTDMT_sizeof_bufferPool(ZSTDMT_bufferPool* bufPool) return poolSize + totalBufferSize; } -static void ZSTDMT_setBufferSize(ZSTDMT_bufferPool* bufPool, size_t bSize) +static void ZSTDMT_setBufferSize(ZSTDMT_bufferPool* const bufPool, size_t const bSize) { ZSTD_pthread_mutex_lock(&bufPool->poolMutex); DEBUGLOG(4, "ZSTDMT_setBufferSize: bSize = %u", (U32)bSize); diff --git a/tests/zstreamtest.c b/tests/zstreamtest.c index b8c437a41..0a09cfa15 100644 --- a/tests/zstreamtest.c +++ b/tests/zstreamtest.c @@ -1486,43 +1486,47 @@ static int fuzzerTests_newAPI(U32 seed, U32 nbTests, unsigned startTest, double cParams = ZSTD_adjustCParams(cParams, 0, 0); if (FUZ_rand(&lseed) & 1) { + DISPLAYLEVEL(5, "t%u: windowLog : %u \n", testNb, cParams.windowLog); CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_windowLog, cParams.windowLog, useOpaqueAPI) ); assert(cParams.windowLog >= ZSTD_WINDOWLOG_MIN); /* guaranteed by ZSTD_adjustCParams() */ windowLogMalus = (cParams.windowLog - ZSTD_WINDOWLOG_MIN) / 5; - DISPLAYLEVEL(5, "t%u: windowLog : %u \n", testNb, cParams.windowLog); } if (FUZ_rand(&lseed) & 1) { - CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_hashLog, cParams.hashLog, useOpaqueAPI) ); DISPLAYLEVEL(5, "t%u: hashLog : %u \n", testNb, cParams.hashLog); + CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_hashLog, cParams.hashLog, useOpaqueAPI) ); } if (FUZ_rand(&lseed) & 1) { - CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_chainLog, cParams.chainLog, useOpaqueAPI) ); DISPLAYLEVEL(5, "t%u: chainLog : %u \n", testNb, cParams.chainLog); + CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_chainLog, cParams.chainLog, useOpaqueAPI) ); } if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_searchLog, cParams.searchLog, useOpaqueAPI) ); if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_minMatch, cParams.searchLength, useOpaqueAPI) ); if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_targetLength, cParams.targetLength, useOpaqueAPI) ); /* mess with long distance matching parameters */ - if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_enableLongDistanceMatching, FUZ_rand(&lseed) & 63, useOpaqueAPI) ); - if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmHashLog, FUZ_randomClampedLength(&lseed, ZSTD_HASHLOG_MIN, 23), useOpaqueAPI) ); - if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmMinMatch, FUZ_randomClampedLength(&lseed, ZSTD_LDM_MINMATCH_MIN, ZSTD_LDM_MINMATCH_MAX), useOpaqueAPI) ); - if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmBucketSizeLog, FUZ_randomClampedLength(&lseed, 0, ZSTD_LDM_BUCKETSIZELOG_MAX), useOpaqueAPI) ); - if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmHashEveryLog, FUZ_randomClampedLength(&lseed, 0, ZSTD_WINDOWLOG_MAX - ZSTD_HASHLOG_MIN), useOpaqueAPI) ); + if (bigTests) { + if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_enableLongDistanceMatching, FUZ_rand(&lseed) & 63, useOpaqueAPI) ); + if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmHashLog, FUZ_randomClampedLength(&lseed, ZSTD_HASHLOG_MIN, 23), useOpaqueAPI) ); + if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmMinMatch, FUZ_randomClampedLength(&lseed, ZSTD_LDM_MINMATCH_MIN, ZSTD_LDM_MINMATCH_MAX), useOpaqueAPI) ); + if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmBucketSizeLog, FUZ_randomClampedLength(&lseed, 0, ZSTD_LDM_BUCKETSIZELOG_MAX), useOpaqueAPI) ); + if (FUZ_rand(&lseed) & 3) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_ldmHashEveryLog, FUZ_randomClampedLength(&lseed, 0, ZSTD_WINDOWLOG_MAX - ZSTD_HASHLOG_MIN), useOpaqueAPI) ); + } /* mess with frame parameters */ if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_checksumFlag, FUZ_rand(&lseed) & 1, useOpaqueAPI) ); if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_dictIDFlag, FUZ_rand(&lseed) & 1, useOpaqueAPI) ); if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_contentSizeFlag, FUZ_rand(&lseed) & 1, useOpaqueAPI) ); - if (FUZ_rand(&lseed) & 1) CHECK_Z( ZSTD_CCtx_setPledgedSrcSize(zc, pledgedSrcSize) ); - DISPLAYLEVEL(5, "t%u: pledgedSrcSize : %u \n", testNb, (U32)pledgedSrcSize); + if (FUZ_rand(&lseed) & 1) { + DISPLAYLEVEL(5, "t%u: pledgedSrcSize : %u \n", testNb, (U32)pledgedSrcSize); + CHECK_Z( ZSTD_CCtx_setPledgedSrcSize(zc, pledgedSrcSize) ); + } /* multi-threading parameters */ { U32 const nbThreadsCandidate = (FUZ_rand(&lseed) & 4) + 1; U32 const nbThreadsAdjusted = (windowLogMalus < nbThreadsCandidate) ? nbThreadsCandidate - windowLogMalus : 1; U32 const nbThreads = MIN(nbThreadsAdjusted, nbThreadsMax); - CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_nbThreads, nbThreads, useOpaqueAPI) ); DISPLAYLEVEL(5, "t%u: nbThreads : %u \n", testNb, nbThreads); + CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_nbThreads, nbThreads, useOpaqueAPI) ); if (nbThreads > 1) { U32 const jobLog = FUZ_rand(&lseed) % (testLog+1); CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_overlapSizeLog, FUZ_rand(&lseed) % 10, useOpaqueAPI) ); @@ -1530,10 +1534,11 @@ static int fuzzerTests_newAPI(U32 seed, U32 nbTests, unsigned startTest, double } } - if (FUZ_rand(&lseed) & 1) CHECK_Z (setCCtxParameter(zc, cctxParams, ZSTD_p_forceMaxWindow, FUZ_rand(&lseed) & 1, useOpaqueAPI) ); + if (FUZ_rand(&lseed) & 1) CHECK_Z( setCCtxParameter(zc, cctxParams, ZSTD_p_forceMaxWindow, FUZ_rand(&lseed) & 1, useOpaqueAPI) ); /* Apply parameters */ if (useOpaqueAPI) { + DISPLAYLEVEL(6," t%u: applying CCtxParams \n", testNb); CHECK_Z (ZSTD_CCtx_setParametersUsingCCtxParams(zc, cctxParams) ); } @@ -1573,8 +1578,8 @@ static int fuzzerTests_newAPI(U32 seed, U32 nbTests, unsigned startTest, double outBuff.size = outBuff.pos + dstBuffSize; CHECK_Z( ZSTD_compress_generic(zc, &outBuff, &inBuff, flush) ); - DISPLAYLEVEL(6, "compress consumed %u bytes (total : %u) \n", - (U32)inBuff.pos, (U32)(totalTestSize + inBuff.pos)); + DISPLAYLEVEL(6, "t%u: compress consumed %u bytes (total : %u) \n", + testNb, (U32)inBuff.pos, (U32)(totalTestSize + inBuff.pos)); XXH64_update(&xxhState, srcBuffer+srcStart, inBuff.pos); memcpy(copyBuffer+totalTestSize, srcBuffer+srcStart, inBuff.pos); From 78290874a5e91e9d9c95ffea67592caa8425ba50 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 29 Nov 2017 17:01:14 -0800 Subject: [PATCH 3/5] fixed Visual warning on minor interface discrepancy --- lib/compress/zstd_compress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index dd12b171b..bc2d8a576 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -470,7 +470,7 @@ size_t ZSTD_CCtxParam_setParameter( * pledgedSrcSize is considered unknown */ size_t ZSTD_CCtx_setParametersUsingCCtxParams( - ZSTD_CCtx* const cctx, const ZSTD_CCtx_params* const params) + ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params) { if (cctx->streamStage != zcss_init) return ERROR(stage_wrong); if (cctx->cdict) return ERROR(stage_wrong); From 5e1f34b7e4c407f7f733c09c435141d18de2dc18 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Fri, 1 Dec 2017 21:17:09 -0800 Subject: [PATCH 4/5] setParameter : no side-effect on setting a compression parameter last such side-effect was modifying cctx->loadedDictEnd on setting forceWindow. It is no a useless operation, so it's removed. No side-effect left when setting a compression parameter. --- lib/compress/zstd_compress.c | 37 ++++------------------------------ lib/compress/zstdmt_compress.c | 2 +- 2 files changed, 5 insertions(+), 34 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index bc2d8a576..8d493852f 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -270,9 +270,8 @@ size_t ZSTD_CCtx_setParameter(ZSTD_CCtx* cctx, ZSTD_cParameter param, unsigned v return ZSTD_CCtxParam_setParameter(&cctx->requestedParams, param, value); case ZSTD_p_forceMaxWindow : /* Force back-references to remain < windowSize, - * even when referencing into Dictionary content + * even when referencing into Dictionary content. * default : 0 when using a CDict, 1 when using a Prefix */ - cctx->loadedDictEnd = 0; /* ? */ return ZSTD_CCtxParam_setParameter(&cctx->requestedParams, param, value); case ZSTD_p_nbThreads: @@ -461,13 +460,9 @@ size_t ZSTD_CCtxParam_setParameter( } } -/** - * This function should be updated whenever ZSTD_CCtx_params is updated. - * Parameters are copied manually before the dictionary is loaded. - * The multithreading parameters jobSize and overlapSizeLog are set only if - * nbThreads > 1. - * - * pledgedSrcSize is considered unknown +/** ZSTD_CCtx_setParametersUsingCCtxParams() : + * just applies `params` into `cctx` + * no action is performed, parameters are merely stored. */ size_t ZSTD_CCtx_setParametersUsingCCtxParams( ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params) @@ -475,31 +470,8 @@ size_t ZSTD_CCtx_setParametersUsingCCtxParams( if (cctx->streamStage != zcss_init) return ERROR(stage_wrong); if (cctx->cdict) return ERROR(stage_wrong); -#if 1 cctx->requestedParams = *params; -#else - /* Assume the compression and frame parameters are validated */ - cctx->requestedParams.cParams = params->cParams; - cctx->requestedParams.fParams = params->fParams; - cctx->requestedParams.compressionLevel = params->compressionLevel; - /* Set force window explicitly since it sets cctx->loadedDictEnd */ - CHECK_F( ZSTD_CCtx_setParameter( - cctx, ZSTD_p_forceMaxWindow, params->forceWindow) ); - - /* Set multithreading parameters explicitly */ - CHECK_F( ZSTD_CCtx_setParameter(cctx, ZSTD_p_nbThreads, params->nbThreads) ); - if (params->nbThreads > 1) { - CHECK_F( ZSTD_CCtx_setParameter(cctx, ZSTD_p_jobSize, params->jobSize) ); - CHECK_F( ZSTD_CCtx_setParameter( - cctx, ZSTD_p_overlapSizeLog, params->overlapSizeLog) ); - } - - /* Copy long distance matching parameters */ - cctx->requestedParams.ldmParams = params->ldmParams; -#endif - - /* customMem is used only for create/free params and can be ignored */ return 0; } @@ -2097,7 +2069,6 @@ size_t ZSTD_compressBegin_advanced_internal( unsigned long long pledgedSrcSize) { DEBUGLOG(4, "ZSTD_compressBegin_advanced_internal"); - DEBUGLOG(4, "contentSizeFlag : %u", params.fParams.contentSizeFlag); /* compression parameters verification and optimization */ CHECK_F( ZSTD_checkCParams(params.cParams) ); return ZSTD_compressBegin_internal(cctx, dict, dictSize, dictMode, NULL, diff --git a/lib/compress/zstdmt_compress.c b/lib/compress/zstdmt_compress.c index ce813dccc..6b6466fe5 100644 --- a/lib/compress/zstdmt_compress.c +++ b/lib/compress/zstdmt_compress.c @@ -359,7 +359,7 @@ void ZSTDMT_compressChunk(void* jobDescription) assert(job->firstChunk); /* should only happen for first segment */ if (ZSTD_isError(initError)) { job->cSize = initError; goto _endJob; } } else { /* srcStart points at reloaded section */ - ZSTD_CCtx_params jobParams = job->params; + ZSTD_CCtx_params jobParams = job->params; /* do not modify job->params ! copy it, modify the copy */ size_t const forceWindowError = ZSTD_CCtxParam_setParameter(&jobParams, ZSTD_p_forceMaxWindow, !job->firstChunk); U64 const pledgedSrcSize = job->firstChunk ? job->fullFrameSize : ZSTD_CONTENTSIZE_UNKNOWN; /* load dictionary in "content-only" mode (no header analysis) */ From 55faa5492d5ba593746868991556e593134e6b57 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Mon, 4 Dec 2017 11:26:59 -0800 Subject: [PATCH 5/5] fileio: fixed LZ4F invocation from assert() --- programs/fileio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/fileio.c b/programs/fileio.c index c9b6b04e1..101c99b1a 100644 --- a/programs/fileio.c +++ b/programs/fileio.c @@ -639,7 +639,6 @@ static unsigned long long FIO_compressLz4Frame(cRess_t* ress, memset(&prefs, 0, sizeof(prefs)); assert(blockSize <= ress->srcBufferSize); - assert(LZ4F_compressBound(blockSize) <= ress->dstBufferSize); prefs.autoFlush = 1; prefs.compressionLevel = compressionLevel; @@ -649,6 +648,7 @@ static unsigned long long FIO_compressLz4Frame(cRess_t* ress, #if LZ4_VERSION_NUMBER >= 10600 prefs.frameInfo.contentSize = (srcFileSize==UTIL_FILESIZE_UNKNOWN) ? 0 : srcFileSize; #endif + assert(LZ4F_compressBound(blockSize, &prefs) <= ress->dstBufferSize); { size_t readSize;