From 17b6da2e0f57f4df3d6034811657d85d24ad831e Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 3 Sep 2019 13:08:24 -0400 Subject: [PATCH 01/21] Track Usable Table Space in Compression Workspace --- lib/compress/zstd_cwksp.h | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/lib/compress/zstd_cwksp.h b/lib/compress/zstd_cwksp.h index 76ace25e..e312e406 100644 --- a/lib/compress/zstd_cwksp.h +++ b/lib/compress/zstd_cwksp.h @@ -133,6 +133,7 @@ typedef struct { void* objectEnd; void* tableEnd; + void* tableValidEnd; void* allocStart; int allocFailed; @@ -161,6 +162,7 @@ MEM_STATIC void ZSTD_cwksp_internal_advance_phase( if (phase > ws->phase) { if (ws->phase < ZSTD_cwksp_alloc_buffers && phase >= ZSTD_cwksp_alloc_buffers) { + ws->tableValidEnd = ws->objectEnd; } if (ws->phase < ZSTD_cwksp_alloc_aligned && phase >= ZSTD_cwksp_alloc_aligned) { @@ -193,6 +195,9 @@ MEM_STATIC void* ZSTD_cwksp_reserve_internal( ws->allocFailed = 1; return NULL; } + if (alloc < ws->tableValidEnd) { + ws->tableValidEnd = alloc; + } ws->allocStart = alloc; return alloc; } @@ -256,9 +261,39 @@ MEM_STATIC void* ZSTD_cwksp_reserve_object(ZSTD_cwksp* ws, size_t bytes) { } ws->objectEnd = end; ws->tableEnd = end; + ws->tableValidEnd = end; return start; } +MEM_STATIC void ZSTD_cwksp_mark_tables_dirty(ZSTD_cwksp* ws) { + DEBUGLOG(4, "cwksp: ZSTD_cwksp_mark_tables_dirty"); + assert(ws->tableValidEnd >= ws->objectEnd); + assert(ws->tableValidEnd <= ws->allocStart); + ws->tableValidEnd = ws->objectEnd; +} + +MEM_STATIC void ZSTD_cwksp_mark_tables_clean(ZSTD_cwksp* ws) { + DEBUGLOG(4, "cwksp: ZSTD_cwksp_mark_tables_clean"); + assert(ws->tableValidEnd >= ws->objectEnd); + assert(ws->tableValidEnd <= ws->allocStart); + if (ws->tableValidEnd < ws->tableEnd) { + ws->tableValidEnd = ws->tableEnd; + } +} + +/** + * Zero the part of the allocated tables not already marked clean. + */ +MEM_STATIC void ZSTD_cwksp_clean_tables(ZSTD_cwksp* ws) { + DEBUGLOG(4, "cwksp: ZSTD_cwksp_clean_tables"); + assert(ws->tableValidEnd >= ws->objectEnd); + assert(ws->tableValidEnd <= ws->allocStart); + if (ws->tableValidEnd < ws->tableEnd) { + memset(ws->tableValidEnd, 0, (BYTE*)ws->tableEnd - (BYTE*)ws->tableValidEnd); + } + ZSTD_cwksp_mark_tables_clean(ws); +} + /** * Invalidates table allocations. * All other allocations remain valid. @@ -293,6 +328,7 @@ MEM_STATIC void ZSTD_cwksp_init(ZSTD_cwksp* ws, void* start, size_t size) { ws->workspace = start; ws->workspaceEnd = (BYTE*)start + size; ws->objectEnd = ws->workspace; + ws->tableValidEnd = ws->objectEnd; ws->phase = ZSTD_cwksp_alloc_objects; ZSTD_cwksp_clear(ws); ws->workspaceOversizedDuration = 0; From 14c5471d5e2307961ffe73b2d4337da071dbed9e Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:24:02 -0400 Subject: [PATCH 02/21] Introduce `ZSTD_indexResetPolicy_e` Enum --- lib/compress/zstd_compress.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index df47ac78..9aaa3eec 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1359,15 +1359,32 @@ static size_t ZSTD_continueCCtx(ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params, return 0; } -typedef enum { ZSTDcrp_continue, ZSTDcrp_noMemset } ZSTD_compResetPolicy_e; +typedef enum { + ZSTDcrp_continue, + ZSTDcrp_noMemset +} ZSTD_compResetPolicy_e; -typedef enum { ZSTD_resetTarget_CDict, ZSTD_resetTarget_CCtx } ZSTD_resetTarget_e; +/** + * Controls, for this matchState reset, whether indexing can continue where it + * left off (ZSTDirp_continue), or whether it needs to be restarted from zero + * (ZSTDirp_reset). + */ +typedef enum { + ZSTDirp_continue, + ZSTDirp_reset +} ZSTD_indexResetPolicy_e; + +typedef enum { + ZSTD_resetTarget_CDict, + ZSTD_resetTarget_CCtx +} ZSTD_resetTarget_e; static size_t ZSTD_reset_matchState(ZSTD_matchState_t* ms, ZSTD_cwksp* ws, const ZSTD_compressionParameters* cParams, - ZSTD_compResetPolicy_e const crp, ZSTD_resetTarget_e const forWho) + const ZSTD_compResetPolicy_e crp, + const ZSTD_resetTarget_e forWho) { size_t const chainSize = (cParams->strategy == ZSTD_fast) ? 0 : ((size_t)1 << cParams->chainLog); size_t const hSize = ((size_t)1) << cParams->hashLog; From 0492b9a9ecee89d8a53a6e009e1a4ce8136a888f Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:28:52 -0400 Subject: [PATCH 03/21] Accept `ZSTD_indexResetPolicy_e` Param in `ZSTD_reset_matchState()` --- lib/compress/zstd_compress.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 9aaa3eec..2474c9f2 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1384,6 +1384,7 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, ZSTD_cwksp* ws, const ZSTD_compressionParameters* cParams, const ZSTD_compResetPolicy_e crp, + const ZSTD_indexResetPolicy_e forceResetIndex, const ZSTD_resetTarget_e forWho) { size_t const chainSize = (cParams->strategy == ZSTD_fast) ? 0 : ((size_t)1 << cParams->chainLog); @@ -1391,6 +1392,7 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, U32 const hashLog3 = ((forWho == ZSTD_resetTarget_CCtx) && cParams->minMatch==3) ? MIN(ZSTD_HASHLOG3_MAX, cParams->windowLog) : 0; size_t const h3Size = ((size_t)1) << hashLog3; + (void)forceResetIndex; ms->hashLog3 = hashLog3; memset(&ms->window, 0, sizeof(ms->window)); @@ -1482,6 +1484,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, ws, ¶ms.cParams, crp, + ZSTDirp_reset, ZSTD_resetTarget_CCtx)); } return ZSTD_continueCCtx(zc, ¶ms, pledgedSrcSize); @@ -1607,7 +1610,9 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, &zc->blockState.matchState, ws, ¶ms.cParams, - crp, ZSTD_resetTarget_CCtx)); + crp, + ZSTDirp_reset, + ZSTD_resetTarget_CCtx)); /* ldm hash table */ /* initialize bucketOffsets table separately for pointer alignment */ @@ -3156,7 +3161,9 @@ static size_t ZSTD_initCDict_internal( &cdict->matchState, &cdict->workspace, &cParams, - ZSTDcrp_continue, ZSTD_resetTarget_CDict)); + ZSTDcrp_continue, + ZSTDirp_reset, + ZSTD_resetTarget_CDict)); /* (Maybe) load the dictionary * Skips loading the dictionary if it is <= 8 bytes. */ From 5b10bb5ec392edb35b5fd25da75cc8113191df83 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:38:32 -0400 Subject: [PATCH 04/21] Rename `ZSTD_compResetPolicy_e` Values and Add Comment --- lib/compress/zstd_compress.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 2474c9f2..7d6e322e 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1359,9 +1359,16 @@ static size_t ZSTD_continueCCtx(ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params, return 0; } +/** + * Controls, for this matchState reset, whether the tables need to be cleared / + * prepared for the coming compression (ZSTDcrp_makeClean), or whether the + * tables can be left unclean (ZSTDcrp_leaveDirty), because we know that a + * subsequent operation will overwrite the table space anyways (e.g., copying + * the matchState contents in from a CDict). + */ typedef enum { - ZSTDcrp_continue, - ZSTDcrp_noMemset + ZSTDcrp_makeClean, + ZSTDcrp_leaveDirty } ZSTD_compResetPolicy_e; /** @@ -1413,8 +1420,8 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, RETURN_ERROR_IF(ZSTD_cwksp_reserve_failed(ws), memory_allocation, "failed a workspace allocation in ZSTD_reset_matchState"); - DEBUGLOG(4, "reset table : %u", crp!=ZSTDcrp_noMemset); - if (crp!=ZSTDcrp_noMemset) { + DEBUGLOG(4, "reset table : %u", crp!=ZSTDcrp_leaveDirty); + if (crp!=ZSTDcrp_leaveDirty) { /* reset tables only */ memset(ms->hashTable, 0, hSize * sizeof(U32)); memset(ms->chainTable, 0, chainSize * sizeof(U32)); @@ -1467,7 +1474,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, assert(!ZSTD_isError(ZSTD_checkCParams(params.cParams))); zc->isFirstBlock = 1; - if (crp == ZSTDcrp_continue) { + if (crp == ZSTDcrp_makeClean) { if (ZSTD_equivalentParams(&zc->appliedParams, ¶ms, zc->inBuffSize, zc->seqStore.maxNbSeq, zc->seqStore.maxNbLit, @@ -1688,7 +1695,7 @@ ZSTD_resetCCtx_byAttachingCDict(ZSTD_CCtx* cctx, params.cParams = ZSTD_adjustCParams_internal(*cdict_cParams, pledgedSrcSize, 0); params.cParams.windowLog = windowLog; FORWARD_IF_ERROR(ZSTD_resetCCtx_internal(cctx, params, pledgedSrcSize, - ZSTDcrp_continue, zbuff)); + ZSTDcrp_makeClean, zbuff)); assert(cctx->appliedParams.cParams.strategy == cdict_cParams->strategy); } @@ -1737,7 +1744,7 @@ static size_t ZSTD_resetCCtx_byCopyingCDict(ZSTD_CCtx* cctx, params.cParams = *cdict_cParams; params.cParams.windowLog = windowLog; FORWARD_IF_ERROR(ZSTD_resetCCtx_internal(cctx, params, pledgedSrcSize, - ZSTDcrp_noMemset, zbuff)); + ZSTDcrp_leaveDirty, zbuff)); assert(cctx->appliedParams.cParams.strategy == cdict_cParams->strategy); assert(cctx->appliedParams.cParams.hashLog == cdict_cParams->hashLog); assert(cctx->appliedParams.cParams.chainLog == cdict_cParams->chainLog); @@ -1820,7 +1827,7 @@ static size_t ZSTD_copyCCtx_internal(ZSTD_CCtx* dstCCtx, params.cParams = srcCCtx->appliedParams.cParams; params.fParams = fParams; ZSTD_resetCCtx_internal(dstCCtx, params, pledgedSrcSize, - ZSTDcrp_noMemset, zbuff); + ZSTDcrp_leaveDirty, zbuff); assert(dstCCtx->appliedParams.cParams.windowLog == srcCCtx->appliedParams.cParams.windowLog); assert(dstCCtx->appliedParams.cParams.strategy == srcCCtx->appliedParams.cParams.strategy); assert(dstCCtx->appliedParams.cParams.hashLog == srcCCtx->appliedParams.cParams.hashLog); @@ -2895,7 +2902,7 @@ static size_t ZSTD_compressBegin_internal(ZSTD_CCtx* cctx, } FORWARD_IF_ERROR( ZSTD_resetCCtx_internal(cctx, *params, pledgedSrcSize, - ZSTDcrp_continue, zbuff) ); + ZSTDcrp_makeClean, zbuff) ); { size_t const dictID = ZSTD_compress_insertDictionary( cctx->blockState.prevCBlock, &cctx->blockState.matchState, params, dict, dictSize, dictContentType, dtlm, @@ -3161,7 +3168,7 @@ static size_t ZSTD_initCDict_internal( &cdict->matchState, &cdict->workspace, &cParams, - ZSTDcrp_continue, + ZSTDcrp_makeClean, ZSTDirp_reset, ZSTD_resetTarget_CDict)); /* (Maybe) load the dictionary From ad16eda5e402de068bb6b6036e94088694c2e82b Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:43:35 -0400 Subject: [PATCH 05/21] `ZSTD_reset_matchState` Optionally Doesn't Restart Indexing --- lib/compress/zstd_compress.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 7d6e322e..ed1eafbe 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1399,13 +1399,16 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, U32 const hashLog3 = ((forWho == ZSTD_resetTarget_CCtx) && cParams->minMatch==3) ? MIN(ZSTD_HASHLOG3_MAX, cParams->windowLog) : 0; size_t const h3Size = ((size_t)1) << hashLog3; - (void)forceResetIndex; + if (forceResetIndex == ZSTDirp_reset) { + memset(&ms->window, 0, sizeof(ms->window)); + ms->window.dictLimit = 1; /* start from 1, so that 1st position is valid */ + ms->window.lowLimit = 1; /* it ensures first and later CCtx usages compress the same */ + ms->window.nextSrc = ms->window.base + 1; /* see issue #1241 */ + ZSTD_cwksp_mark_tables_dirty(ws); + } ms->hashLog3 = hashLog3; - memset(&ms->window, 0, sizeof(ms->window)); - ms->window.dictLimit = 1; /* start from 1, so that 1st position is valid */ - ms->window.lowLimit = 1; /* it ensures first and later CCtx usages compress the same */ - ms->window.nextSrc = ms->window.base + 1; /* see issue #1241 */ + ZSTD_invalidateMatchState(ms); assert(!ZSTD_cwksp_reserve_failed(ws)); /* check that allocation hasn't already failed */ @@ -1423,9 +1426,7 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, DEBUGLOG(4, "reset table : %u", crp!=ZSTDcrp_leaveDirty); if (crp!=ZSTDcrp_leaveDirty) { /* reset tables only */ - memset(ms->hashTable, 0, hSize * sizeof(U32)); - memset(ms->chainTable, 0, chainSize * sizeof(U32)); - memset(ms->hashTable3, 0, h3Size * sizeof(U32)); + ZSTD_cwksp_clean_tables(ws); } /* opt parser space */ From 1b28e804163231956ec22d78e861682f29c741be Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:54:11 -0400 Subject: [PATCH 06/21] Remove Fast Continue Path in `ZSTD_resetCCtx_internal()` --- lib/compress/zstd_compress.c | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index ed1eafbe..af9ecad2 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1475,29 +1475,6 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, assert(!ZSTD_isError(ZSTD_checkCParams(params.cParams))); zc->isFirstBlock = 1; - if (crp == ZSTDcrp_makeClean) { - if (ZSTD_equivalentParams(&zc->appliedParams, ¶ms, - zc->inBuffSize, - zc->seqStore.maxNbSeq, zc->seqStore.maxNbLit, - zbuff, pledgedSrcSize) ) { - DEBUGLOG(4, "ZSTD_equivalentParams()==1 -> consider continue mode"); - ZSTD_cwksp_bump_oversized_duration(ws, 0); - if (!ZSTD_cwksp_check_wasteful(ws, 0)) { - DEBUGLOG(4, "continue mode confirmed (wLog1=%u, blockSize1=%zu)", - zc->appliedParams.cParams.windowLog, zc->blockSize); - if (ZSTD_indexTooCloseToMax(zc->blockState.matchState.window)) { - /* prefer a reset, faster than a rescale */ - FORWARD_IF_ERROR(ZSTD_reset_matchState( - &zc->blockState.matchState, - ws, - ¶ms.cParams, - crp, - ZSTDirp_reset, - ZSTD_resetTarget_CCtx)); - } - return ZSTD_continueCCtx(zc, ¶ms, pledgedSrcSize); - } } } - DEBUGLOG(4, "ZSTD_equivalentParams()==0 -> reset CCtx"); if (params.ldmParams.enableLdm) { /* Adjust long distance matching parameters */ From 9968a53e912aae109aae61c08cb8fb07f9980471 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:59:45 -0400 Subject: [PATCH 07/21] Remove No-Longer-Used Continuation Functions --- lib/compress/zstd_compress.c | 114 +++-------------------------------- 1 file changed, 9 insertions(+), 105 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index af9ecad2..21b13fa8 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1214,17 +1214,6 @@ size_t ZSTD_toFlushNow(ZSTD_CCtx* cctx) return 0; /* over-simplification; could also check if context is currently running in streaming mode, and in which case, report how many bytes are left to be flushed within output buffer */ } - - -static U32 ZSTD_equivalentCParams(ZSTD_compressionParameters cParams1, - ZSTD_compressionParameters cParams2) -{ - return (cParams1.hashLog == cParams2.hashLog) - & (cParams1.chainLog == cParams2.chainLog) - & (cParams1.strategy == cParams2.strategy) /* opt parser space */ - & ((cParams1.minMatch==3) == (cParams2.minMatch==3)); /* hashlog3 space */ -} - static void ZSTD_assertEqualCParams(ZSTD_compressionParameters cParams1, ZSTD_compressionParameters cParams2) { @@ -1239,71 +1228,6 @@ static void ZSTD_assertEqualCParams(ZSTD_compressionParameters cParams1, assert(cParams1.strategy == cParams2.strategy); } -/** The parameters are equivalent if ldm is not enabled in both sets or - * all the parameters are equivalent. */ -static U32 ZSTD_equivalentLdmParams(ldmParams_t ldmParams1, - ldmParams_t ldmParams2) -{ - return (!ldmParams1.enableLdm && !ldmParams2.enableLdm) || - (ldmParams1.enableLdm == ldmParams2.enableLdm && - ldmParams1.hashLog == ldmParams2.hashLog && - ldmParams1.bucketSizeLog == ldmParams2.bucketSizeLog && - ldmParams1.minMatchLength == ldmParams2.minMatchLength && - ldmParams1.hashRateLog == ldmParams2.hashRateLog); -} - -typedef enum { ZSTDb_not_buffered, ZSTDb_buffered } ZSTD_buffered_policy_e; - -/* ZSTD_sufficientBuff() : - * check internal buffers exist for streaming if buffPol == ZSTDb_buffered . - * Note : they are assumed to be correctly sized if ZSTD_equivalentCParams()==1 */ -static U32 ZSTD_sufficientBuff(size_t bufferSize1, size_t maxNbSeq1, - size_t maxNbLit1, - ZSTD_buffered_policy_e buffPol2, - ZSTD_compressionParameters cParams2, - U64 pledgedSrcSize) -{ - size_t const windowSize2 = MAX(1, (size_t)MIN(((U64)1 << cParams2.windowLog), pledgedSrcSize)); - size_t const blockSize2 = MIN(ZSTD_BLOCKSIZE_MAX, windowSize2); - size_t const maxNbSeq2 = blockSize2 / ((cParams2.minMatch == 3) ? 3 : 4); - size_t const maxNbLit2 = blockSize2; - size_t const neededBufferSize2 = (buffPol2==ZSTDb_buffered) ? windowSize2 + blockSize2 : 0; - DEBUGLOG(4, "ZSTD_sufficientBuff: is neededBufferSize2=%u <= bufferSize1=%u", - (U32)neededBufferSize2, (U32)bufferSize1); - DEBUGLOG(4, "ZSTD_sufficientBuff: is maxNbSeq2=%u <= maxNbSeq1=%u", - (U32)maxNbSeq2, (U32)maxNbSeq1); - DEBUGLOG(4, "ZSTD_sufficientBuff: is maxNbLit2=%u <= maxNbLit1=%u", - (U32)maxNbLit2, (U32)maxNbLit1); - return (maxNbLit2 <= maxNbLit1) - & (maxNbSeq2 <= maxNbSeq1) - & (neededBufferSize2 <= bufferSize1); -} - -/** Equivalence for resetCCtx purposes */ -static U32 ZSTD_equivalentParams(const ZSTD_CCtx_params* params1, - const ZSTD_CCtx_params* params2, - size_t buffSize1, - size_t maxNbSeq1, size_t maxNbLit1, - ZSTD_buffered_policy_e buffPol2, - U64 pledgedSrcSize) -{ - DEBUGLOG(4, "ZSTD_equivalentParams: pledgedSrcSize=%u", (U32)pledgedSrcSize); - if (!ZSTD_equivalentCParams(params1->cParams, params2->cParams)) { - DEBUGLOG(4, "ZSTD_equivalentCParams() == 0"); - return 0; - } - if (!ZSTD_equivalentLdmParams(params1->ldmParams, params2->ldmParams)) { - DEBUGLOG(4, "ZSTD_equivalentLdmParams() == 0"); - return 0; - } - if (!ZSTD_sufficientBuff(buffSize1, maxNbSeq1, maxNbLit1, buffPol2, - params2->cParams, pledgedSrcSize)) { - DEBUGLOG(4, "ZSTD_sufficientBuff() == 0"); - return 0; - } - return 1; -} - static void ZSTD_reset_compressedBlockState(ZSTD_compressedBlockState_t* bs) { int i; @@ -1329,35 +1253,15 @@ static void ZSTD_invalidateMatchState(ZSTD_matchState_t* ms) ms->dictMatchState = NULL; } -/*! ZSTD_continueCCtx() : - * reuse CCtx without reset (note : requires no dictionary) */ -static size_t ZSTD_continueCCtx(ZSTD_CCtx* cctx, const ZSTD_CCtx_params* params, U64 pledgedSrcSize) -{ - size_t const windowSize = MAX(1, (size_t)MIN(((U64)1 << params->cParams.windowLog), pledgedSrcSize)); - size_t const blockSize = MIN(ZSTD_BLOCKSIZE_MAX, windowSize); - DEBUGLOG(4, "ZSTD_continueCCtx: re-use context in place"); - - cctx->blockSize = blockSize; /* previous block size could be different even for same windowLog, due to pledgedSrcSize */ - cctx->appliedParams = *params; - cctx->blockState.matchState.cParams = params->cParams; - cctx->pledgedSrcSizePlusOne = pledgedSrcSize+1; - cctx->consumedSrcSize = 0; - cctx->isFirstBlock = 1; - cctx->producedCSize = 0; - if (pledgedSrcSize == ZSTD_CONTENTSIZE_UNKNOWN) - cctx->appliedParams.fParams.contentSizeFlag = 0; - DEBUGLOG(4, "pledged content size : %u ; flag : %u", - (U32)pledgedSrcSize, cctx->appliedParams.fParams.contentSizeFlag); - cctx->stage = ZSTDcs_init; - cctx->dictID = 0; - if (params->ldmParams.enableLdm) - ZSTD_window_clear(&cctx->ldmState.window); - ZSTD_referenceExternalSequences(cctx, NULL, 0); - ZSTD_invalidateMatchState(&cctx->blockState.matchState); - ZSTD_reset_compressedBlockState(cctx->blockState.prevCBlock); - XXH64_reset(&cctx->xxhState, 0); - return 0; -} +/** + * Indicates whether this compression proceeds directly from user-provided + * source buffer to user-provided destination buffer (ZSTDb_not_buffered), or + * whether the context needs to buffer the input/output (ZSTDb_buffered). + */ +typedef enum { + ZSTDb_not_buffered, + ZSTDb_buffered +} ZSTD_buffered_policy_e; /** * Controls, for this matchState reset, whether the tables need to be cleared / From f31ef28ff894c9ccdbac1dbc611cb9e702d71738 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 17:55:41 -0400 Subject: [PATCH 08/21] Only Reset Indexing in `ZSTD_resetCCtx_internal()` When Necessary --- lib/compress/zstd_compress.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 21b13fa8..ec7e7836 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1398,6 +1398,14 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, size_t const matchStateSize = ZSTD_sizeof_matchState(¶ms.cParams, /* forCCtx */ 1); size_t const maxNbLdmSeq = ZSTD_ldm_getMaxNbSeq(params.ldmParams, blockSize); + ZSTD_indexResetPolicy_e needsIndexReset = ZSTDirp_continue; + + if (ZSTD_indexTooCloseToMax(zc->blockState.matchState.window)) { + needsIndexReset = ZSTDirp_reset; + } + + ZSTD_cwksp_bump_oversized_duration(ws, 0); + /* Check if workspace is large enough, alloc a new one if needed */ { size_t const cctxSpace = zc->staticSize ? sizeof(ZSTD_CCtx) : 0; size_t const entropySpace = HUF_WORKSPACE_SIZE; @@ -1430,6 +1438,8 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, RETURN_ERROR_IF(zc->staticSize, memory_allocation, "static cctx : no resize"); + needsIndexReset = ZSTDirp_reset; + ZSTD_cwksp_free(ws, zc->customMem); FORWARD_IF_ERROR(ZSTD_cwksp_create(ws, neededSpace, zc->customMem)); @@ -1500,7 +1510,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, ws, ¶ms.cParams, crp, - ZSTDirp_reset, + needsIndexReset, ZSTD_resetTarget_CCtx)); /* ldm hash table */ From edb3ad053eb0784b6bc7e03778758b513dc23852 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Tue, 10 Sep 2019 18:02:22 -0400 Subject: [PATCH 09/21] Comments --- lib/compress/zstd_compress.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index ec7e7836..3fb77d59 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1490,6 +1490,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, /* ldm bucketOffsets table */ if (params.ldmParams.enableLdm) { + /* TODO: avoid memset? */ size_t const ldmBucketSize = ((size_t)1) << (params.ldmParams.hashLog - params.ldmParams.bucketSizeLog); @@ -1514,8 +1515,8 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc, ZSTD_resetTarget_CCtx)); /* ldm hash table */ - /* initialize bucketOffsets table separately for pointer alignment */ if (params.ldmParams.enableLdm) { + /* TODO: avoid memset? */ size_t const ldmHSize = ((size_t)1) << params.ldmParams.hashLog; zc->ldmState.hashTable = (ldmEntry_t*)ZSTD_cwksp_reserve_aligned(ws, ldmHSize * sizeof(ldmEntry_t)); memset(zc->ldmState.hashTable, 0, ldmHSize * sizeof(ldmEntry_t)); From 13e29a56de32e649e37c87ffc64f11e6be55edc1 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 11:18:45 -0400 Subject: [PATCH 10/21] Shrink Clean Table Area When Copying Table Contents into Context The source matchState is potentially at a lower current index, which means that any extra table space not overwritten by the copy may now contain invalid indices. The simple solution is to unconditionally shrink the valid table area to just the area overwritten. --- lib/compress/zstd_compress.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 3fb77d59..64fe952a 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1643,6 +1643,8 @@ static size_t ZSTD_resetCCtx_byCopyingCDict(ZSTD_CCtx* cctx, assert(cctx->appliedParams.cParams.chainLog == cdict_cParams->chainLog); } + ZSTD_cwksp_mark_tables_dirty(&cctx->workspace); + /* copy tables */ { size_t const chainSize = (cdict_cParams->strategy == ZSTD_fast) ? 0 : ((size_t)1 << cdict_cParams->chainLog); size_t const hSize = (size_t)1 << cdict_cParams->hashLog; @@ -1660,6 +1662,8 @@ static size_t ZSTD_resetCCtx_byCopyingCDict(ZSTD_CCtx* cctx, memset(cctx->blockState.matchState.hashTable3, 0, h3Size * sizeof(U32)); } + ZSTD_cwksp_mark_tables_clean(&cctx->workspace); + /* copy dictionary offsets */ { ZSTD_matchState_t const* srcMatchState = &cdict->matchState; ZSTD_matchState_t* dstMatchState = &cctx->blockState.matchState; @@ -1728,6 +1732,8 @@ static size_t ZSTD_copyCCtx_internal(ZSTD_CCtx* dstCCtx, assert(dstCCtx->blockState.matchState.hashLog3 == srcCCtx->blockState.matchState.hashLog3); } + ZSTD_cwksp_mark_tables_dirty(&dstCCtx->workspace); + /* copy tables */ { size_t const chainSize = (srcCCtx->appliedParams.cParams.strategy == ZSTD_fast) ? 0 : ((size_t)1 << srcCCtx->appliedParams.cParams.chainLog); size_t const hSize = (size_t)1 << srcCCtx->appliedParams.cParams.hashLog; @@ -1738,6 +1744,8 @@ static size_t ZSTD_copyCCtx_internal(ZSTD_CCtx* dstCCtx, memcpy(dstCCtx->blockState.matchState.hashTable, srcCCtx->blockState.matchState.hashTable, tableSpace); /* presumes all tables follow each other */ } + ZSTD_cwksp_mark_tables_clean(&dstCCtx->workspace); + /* copy dictionary offsets */ { const ZSTD_matchState_t* srcMatchState = &srcCCtx->blockState.matchState; From 1999b2ed9bdae1c0e6b5a6f976c8a67dc33d133d Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 11:21:00 -0400 Subject: [PATCH 11/21] Update DEBUGLOG Statements --- lib/compress/zstd_compress.c | 1 + lib/compress/zstd_cwksp.h | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 64fe952a..28341c0d 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1303,6 +1303,7 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, U32 const hashLog3 = ((forWho == ZSTD_resetTarget_CCtx) && cParams->minMatch==3) ? MIN(ZSTD_HASHLOG3_MAX, cParams->windowLog) : 0; size_t const h3Size = ((size_t)1) << hashLog3; + DEBUGLOG(4, "reset indices : %u", forceResetIndex == ZSTDirp_reset); if (forceResetIndex == ZSTDirp_reset) { memset(&ms->window, 0, sizeof(ms->window)); ms->window.dictLimit = 1; /* start from 1, so that 1st position is valid */ diff --git a/lib/compress/zstd_cwksp.h b/lib/compress/zstd_cwksp.h index e312e406..4c146145 100644 --- a/lib/compress/zstd_cwksp.h +++ b/lib/compress/zstd_cwksp.h @@ -188,7 +188,7 @@ MEM_STATIC void* ZSTD_cwksp_reserve_internal( void* bottom = ws->tableEnd; ZSTD_cwksp_internal_advance_phase(ws, phase); alloc = (BYTE *)ws->allocStart - bytes; - DEBUGLOG(4, "cwksp: reserving %zd bytes, %zd bytes remaining", + DEBUGLOG(5, "cwksp: reserving %zd bytes, %zd bytes remaining", bytes, ZSTD_cwksp_available_space(ws) - bytes); assert(alloc >= bottom); if (alloc < bottom) { @@ -227,7 +227,7 @@ MEM_STATIC void* ZSTD_cwksp_reserve_table(ZSTD_cwksp* ws, size_t bytes) { void* alloc = ws->tableEnd; void* end = (BYTE *)alloc + bytes; void* top = ws->allocStart; - DEBUGLOG(4, "cwksp: reserving table %zd bytes, %zd bytes remaining", + DEBUGLOG(5, "cwksp: reserving table %zd bytes, %zd bytes remaining", bytes, ZSTD_cwksp_available_space(ws) - bytes); assert((bytes & (sizeof(U32)-1)) == 0); ZSTD_cwksp_internal_advance_phase(ws, phase); @@ -248,7 +248,7 @@ MEM_STATIC void* ZSTD_cwksp_reserve_object(ZSTD_cwksp* ws, size_t bytes) { size_t roundedBytes = ZSTD_cwksp_align(bytes, sizeof(void*)); void* start = ws->objectEnd; void* end = (BYTE*)start + roundedBytes; - DEBUGLOG(4, + DEBUGLOG(5, "cwksp: reserving object %zd bytes (rounded to %zd), %zd bytes remaining", bytes, roundedBytes, ZSTD_cwksp_available_space(ws) - roundedBytes); assert(((size_t)start & (sizeof(void*)-1)) == 0); From bc020eec928b09c2cf99712d5b4a00afa34470de Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 11:40:57 -0400 Subject: [PATCH 12/21] Also Shrink Clean Table Area When Reducing Indices --- lib/compress/zstd_compress.c | 37 +++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 28341c0d..e0ffbc7c 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2320,7 +2320,11 @@ out: } -static void ZSTD_overflowCorrectIfNeeded(ZSTD_matchState_t* ms, ZSTD_CCtx_params const* params, void const* ip, void const* iend) +static void ZSTD_overflowCorrectIfNeeded(ZSTD_matchState_t* ms, + ZSTD_cwksp* ws, + ZSTD_CCtx_params const* params, + void const* ip, + void const* iend) { if (ZSTD_window_needOverflowCorrection(ms->window, iend)) { U32 const maxDist = (U32)1 << params->cParams.windowLog; @@ -2329,7 +2333,9 @@ static void ZSTD_overflowCorrectIfNeeded(ZSTD_matchState_t* ms, ZSTD_CCtx_params ZSTD_STATIC_ASSERT(ZSTD_CHAINLOG_MAX <= 30); ZSTD_STATIC_ASSERT(ZSTD_WINDOWLOG_MAX_32 <= 30); ZSTD_STATIC_ASSERT(ZSTD_WINDOWLOG_MAX <= 31); + ZSTD_cwksp_mark_tables_dirty(ws); ZSTD_reduceIndex(ms, params, correction); + ZSTD_cwksp_mark_tables_clean(ws); if (ms->nextToUpdate < correction) ms->nextToUpdate = 0; else ms->nextToUpdate -= correction; /* invalidate dictionaries on overflow correction */ @@ -2372,7 +2378,8 @@ static size_t ZSTD_compress_frameChunk (ZSTD_CCtx* cctx, "not enough space to store compressed block"); if (remaining < blockSize) blockSize = remaining; - ZSTD_overflowCorrectIfNeeded(ms, &cctx->appliedParams, ip, ip + blockSize); + ZSTD_overflowCorrectIfNeeded( + ms, &cctx->workspace, &cctx->appliedParams, ip, ip + blockSize); ZSTD_checkDictValidity(&ms->window, ip + blockSize, maxDist, &ms->loadedDictEnd, &ms->dictMatchState); /* Ensure hash/chain table insertion resumes no sooner than lowlimit */ @@ -2515,7 +2522,9 @@ static size_t ZSTD_compressContinue_internal (ZSTD_CCtx* cctx, if (!frame) { /* overflow check and correction for block mode */ - ZSTD_overflowCorrectIfNeeded(ms, &cctx->appliedParams, src, (BYTE const*)src + srcSize); + ZSTD_overflowCorrectIfNeeded( + ms, &cctx->workspace, &cctx->appliedParams, + src, (BYTE const*)src + srcSize); } DEBUGLOG(5, "ZSTD_compressContinue_internal (blockSize=%u)", (unsigned)cctx->blockSize); @@ -2568,6 +2577,7 @@ size_t ZSTD_compressBlock(ZSTD_CCtx* cctx, void* dst, size_t dstCapacity, const * @return : 0, or an error code */ static size_t ZSTD_loadDictionaryContent(ZSTD_matchState_t* ms, + ZSTD_cwksp* ws, ZSTD_CCtx_params const* params, const void* src, size_t srcSize, ZSTD_dictTableLoadMethod_e dtlm) @@ -2588,7 +2598,7 @@ static size_t ZSTD_loadDictionaryContent(ZSTD_matchState_t* ms, size_t const chunk = MIN(remaining, ZSTD_CHUNKSIZE_MAX); const BYTE* const ichunk = ip + chunk; - ZSTD_overflowCorrectIfNeeded(ms, params, ip, ichunk); + ZSTD_overflowCorrectIfNeeded(ms, ws, params, ip, ichunk); switch(params->cParams.strategy) { @@ -2651,6 +2661,7 @@ static size_t ZSTD_checkDictNCount(short* normalizedCounter, unsigned dictMaxSym */ static size_t ZSTD_loadZstdDictionary(ZSTD_compressedBlockState_t* bs, ZSTD_matchState_t* ms, + ZSTD_cwksp* ws, ZSTD_CCtx_params const* params, const void* dict, size_t dictSize, ZSTD_dictTableLoadMethod_e dtlm, @@ -2746,7 +2757,8 @@ static size_t ZSTD_loadZstdDictionary(ZSTD_compressedBlockState_t* bs, bs->entropy.fse.offcode_repeatMode = FSE_repeat_valid; bs->entropy.fse.matchlength_repeatMode = FSE_repeat_valid; bs->entropy.fse.litlength_repeatMode = FSE_repeat_valid; - FORWARD_IF_ERROR(ZSTD_loadDictionaryContent(ms, params, dictPtr, dictContentSize, dtlm)); + FORWARD_IF_ERROR(ZSTD_loadDictionaryContent( + ms, ws, params, dictPtr, dictContentSize, dtlm)); return dictID; } } @@ -2756,6 +2768,7 @@ static size_t ZSTD_loadZstdDictionary(ZSTD_compressedBlockState_t* bs, static size_t ZSTD_compress_insertDictionary(ZSTD_compressedBlockState_t* bs, ZSTD_matchState_t* ms, + ZSTD_cwksp* ws, const ZSTD_CCtx_params* params, const void* dict, size_t dictSize, ZSTD_dictContentType_e dictContentType, @@ -2769,19 +2782,21 @@ ZSTD_compress_insertDictionary(ZSTD_compressedBlockState_t* bs, /* dict restricted modes */ if (dictContentType == ZSTD_dct_rawContent) - return ZSTD_loadDictionaryContent(ms, params, dict, dictSize, dtlm); + return ZSTD_loadDictionaryContent(ms, ws, params, dict, dictSize, dtlm); if (MEM_readLE32(dict) != ZSTD_MAGIC_DICTIONARY) { if (dictContentType == ZSTD_dct_auto) { DEBUGLOG(4, "raw content dictionary detected"); - return ZSTD_loadDictionaryContent(ms, params, dict, dictSize, dtlm); + return ZSTD_loadDictionaryContent( + ms, ws, params, dict, dictSize, dtlm); } RETURN_ERROR_IF(dictContentType == ZSTD_dct_fullDict, dictionary_wrong); assert(0); /* impossible */ } /* dict as full zstd dictionary */ - return ZSTD_loadZstdDictionary(bs, ms, params, dict, dictSize, dtlm, workspace); + return ZSTD_loadZstdDictionary( + bs, ms, ws, params, dict, dictSize, dtlm, workspace); } /*! ZSTD_compressBegin_internal() : @@ -2807,7 +2822,7 @@ static size_t ZSTD_compressBegin_internal(ZSTD_CCtx* cctx, ZSTDcrp_makeClean, zbuff) ); { size_t const dictID = ZSTD_compress_insertDictionary( cctx->blockState.prevCBlock, &cctx->blockState.matchState, - params, dict, dictSize, dictContentType, dtlm, + &cctx->workspace, params, dict, dictSize, dictContentType, dtlm, cctx->entropyWorkspace); FORWARD_IF_ERROR(dictID); assert(dictID <= UINT_MAX); @@ -3082,8 +3097,8 @@ static size_t ZSTD_initCDict_internal( params.fParams.contentSizeFlag = 1; params.cParams = cParams; { size_t const dictID = ZSTD_compress_insertDictionary( - &cdict->cBlockState, &cdict->matchState, ¶ms, - cdict->dictContent, cdict->dictContentSize, + &cdict->cBlockState, &cdict->matchState, &cdict->workspace, + ¶ms, cdict->dictContent, cdict->dictContentSize, dictContentType, ZSTD_dtlm_full, cdict->entropyWorkspace); FORWARD_IF_ERROR(dictID); assert(dictID <= (size_t)(U32)-1); From 7c57e2b9cac597845265f877c8c866bf38b7ce01 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 13:14:26 -0400 Subject: [PATCH 13/21] Zero `h3size` When `h3log` is 0 This led to a nasty edgecase, where index reduction for modes that don't use the h3 table would have a degenerate table (size 4) allocated and marked clean, but which would not be re-indexed. --- lib/compress/zstd_compress.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index e0ffbc7c..0d2e9440 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1301,7 +1301,7 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, size_t const chainSize = (cParams->strategy == ZSTD_fast) ? 0 : ((size_t)1 << cParams->chainLog); size_t const hSize = ((size_t)1) << cParams->hashLog; U32 const hashLog3 = ((forWho == ZSTD_resetTarget_CCtx) && cParams->minMatch==3) ? MIN(ZSTD_HASHLOG3_MAX, cParams->windowLog) : 0; - size_t const h3Size = ((size_t)1) << hashLog3; + size_t const h3Size = hashLog3 ? ((size_t)1) << hashLog3 : 0; DEBUGLOG(4, "reset indices : %u", forceResetIndex == ZSTDirp_reset); if (forceResetIndex == ZSTDirp_reset) { @@ -1658,7 +1658,8 @@ static size_t ZSTD_resetCCtx_byCopyingCDict(ZSTD_CCtx* cctx, } /* Zero the hashTable3, since the cdict never fills it */ - { size_t const h3Size = (size_t)1 << cctx->blockState.matchState.hashLog3; + { int const h3log = cctx->blockState.matchState.hashLog3; + size_t const h3Size = h3log ? ((size_t)1 << h3log) : 0; assert(cdict->matchState.hashLog3 == 0); memset(cctx->blockState.matchState.hashTable3, 0, h3Size * sizeof(U32)); } @@ -1738,7 +1739,8 @@ static size_t ZSTD_copyCCtx_internal(ZSTD_CCtx* dstCCtx, /* copy tables */ { size_t const chainSize = (srcCCtx->appliedParams.cParams.strategy == ZSTD_fast) ? 0 : ((size_t)1 << srcCCtx->appliedParams.cParams.chainLog); size_t const hSize = (size_t)1 << srcCCtx->appliedParams.cParams.hashLog; - size_t const h3Size = (size_t)1 << srcCCtx->blockState.matchState.hashLog3; + int const h3log = srcCCtx->blockState.matchState.hashLog3; + size_t const h3Size = h3log ? ((size_t)1 << h3log) : 0; size_t const tableSpace = (chainSize + hSize + h3Size) * sizeof(U32); assert((U32*)dstCCtx->blockState.matchState.chainTable == (U32*)dstCCtx->blockState.matchState.hashTable + hSize); /* chainTable must follow hashTable */ assert((U32*)dstCCtx->blockState.matchState.hashTable3 == (U32*)dstCCtx->blockState.matchState.chainTable + chainSize); From ed4c2c60c342db07c16746188b71aad217f70f93 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 13:17:19 -0400 Subject: [PATCH 14/21] Add Fuzzer Test Case for Index Reduction --- tests/fuzzer.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index fe656e6c..53bcb063 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2150,6 +2150,78 @@ static int basicUnitTests(U32 const seed, double compressibility) } DISPLAYLEVEL(3, "OK \n"); + DISPLAYLEVEL(3, "test%3i : table cleanliness through index reduction : ", testNb++); + { + int cLevel; + uint32_t approxIndex = 0; + uint32_t maxIndex = ((3U << 29) + (1U << ZSTD_WINDOWLOG_MAX)); /* ZSTD_CURRENT_MAX from zstd_compress_internal.h */ + + /* vastly overprovision space in a static context so that we can do all + * this without ever reallocating, which would reset the indices */ + size_t const staticCCtxSize = 2 * ZSTD_estimateCCtxSize(22); + void* const staticCCtxBuffer = malloc(staticCCtxSize); + ZSTD_CCtx* cctx = ZSTD_initStaticCCtx(staticCCtxBuffer, staticCCtxSize); + + /* bump the indices so the following compressions happen at high + * indices. */ + { + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; + ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, -500)); + while (approxIndex <= (maxIndex / 4) * 3) { + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_flush)); + approxIndex += in.pos; + CHECK(in.pos == in.size); + in.pos = 0; + out.pos = 0; + } + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end)); + } + + /* spew a bunch of stuff into the table area */ + for (cLevel = 1; cLevel <= 22; cLevel++) { + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; + ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, cLevel)); + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_flush)); + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end)); + approxIndex += in.pos; + } + + /* now crank the indices so we overflow */ + { + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; + ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, -500)); + while (approxIndex <= maxIndex) { + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_flush)); + approxIndex += in.pos; + CHECK(in.pos == in.size); + in.pos = 0; + out.pos = 0; + } + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end)); + } + + /* do a bunch of compressions again in low indices and ensure we don't + * hit untracked invalid indices */ + for (cLevel = 1; cLevel <= 22; cLevel++) { + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; + ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, cLevel)); + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_flush)); + CHECK_Z(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end)); + approxIndex += in.pos; + } + + ZSTD_freeCCtx(cctx); + } + DISPLAYLEVEL(3, "OK \n"); + _end: free(CNBuffer); free(compressedBuffer); From 5707c8a9d5f9b8ec89467862d42561e811c194a2 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 13:23:59 -0400 Subject: [PATCH 15/21] Speed Up Test a Little --- tests/fuzzer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 53bcb063..cfe477d3 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2181,7 +2181,7 @@ static int basicUnitTests(U32 const seed, double compressibility) /* spew a bunch of stuff into the table area */ for (cLevel = 1; cLevel <= 22; cLevel++) { - ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize / cLevel, 0 }; ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, cLevel)); @@ -2209,7 +2209,7 @@ static int basicUnitTests(U32 const seed, double compressibility) /* do a bunch of compressions again in low indices and ensure we don't * hit untracked invalid indices */ for (cLevel = 1; cLevel <= 22; cLevel++) { - ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize, 0 }; + ZSTD_outBuffer out = { compressedBuffer, compressedBufferSize / cLevel, 0 }; ZSTD_inBuffer in = { CNBuffer, CNBuffSize, 0 }; ZSTD_CCtx_reset(cctx, ZSTD_reset_session_and_parameters); CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, cLevel)); From ff67c62458869928680cb6c856299214f75d6d94 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 13:59:09 -0400 Subject: [PATCH 16/21] Fix Compilation Error (`uint32_t` -> `size_t`) --- tests/fuzzer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index cfe477d3..0956b9c5 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2153,8 +2153,8 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "test%3i : table cleanliness through index reduction : ", testNb++); { int cLevel; - uint32_t approxIndex = 0; - uint32_t maxIndex = ((3U << 29) + (1U << ZSTD_WINDOWLOG_MAX)); /* ZSTD_CURRENT_MAX from zstd_compress_internal.h */ + size_t approxIndex = 0; + size_t maxIndex = ((3U << 29) + (1U << ZSTD_WINDOWLOG_MAX)); /* ZSTD_CURRENT_MAX from zstd_compress_internal.h */ /* vastly overprovision space in a static context so that we can do all * this without ever reallocating, which would reset the indices */ From 194c542598a07ce13978d9306777e35bf4dcf3f7 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 14:25:30 -0400 Subject: [PATCH 17/21] Fix Memory Leak in Test --- tests/fuzzer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 0956b9c5..0ae0b394 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2219,6 +2219,7 @@ static int basicUnitTests(U32 const seed, double compressibility) } ZSTD_freeCCtx(cctx); + free(staticCCtxBuffer); } DISPLAYLEVEL(3, "OK \n"); From a10c1916130625b9a9cb2534a0a09109bcdbd2a9 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 16:40:29 -0400 Subject: [PATCH 18/21] `__msan_poison()` Workspace When Preparing for Re-Use --- lib/common/mem.h | 17 +++++++++++++++++ lib/compress/zstd_compress.c | 14 ++++++++++++++ lib/compress/zstd_cwksp.h | 24 ++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/lib/common/mem.h b/lib/common/mem.h index c10d7f61..3dc4f428 100644 --- a/lib/common/mem.h +++ b/lib/common/mem.h @@ -47,6 +47,23 @@ extern "C" { #define MEM_STATIC_ASSERT(c) { enum { MEM_static_assert = 1/(int)(!!(c)) }; } MEM_STATIC void MEM_check(void) { MEM_STATIC_ASSERT((sizeof(size_t)==4) || (sizeof(size_t)==8)); } +/* detects whether we are being compiled under msan */ +#if defined (__has_feature) +# if __has_feature(memory_sanitizer) +# define MEMORY_SANITIZER 1 +# endif +#endif + +#if defined (MEMORY_SANITIZER) +# include +#endif + +#if defined (MEMORY_SANITIZER) +# define MEM_SKIP_MSAN __attribute__((no_sanitize("memory"))) +#else +# define MEM_SKIP_MSAN +#endif + /*-************************************************************** * Basic Types diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 0d2e9440..2f0736b2 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -1799,6 +1799,20 @@ ZSTD_reduceTable_internal (U32* const table, U32 const size, U32 const reducerVa int rowNb; assert((size & (ZSTD_ROWSIZE-1)) == 0); /* multiple of ZSTD_ROWSIZE */ assert(size < (1U<<31)); /* can be casted to int */ + +#if defined (MEMORY_SANITIZER) && !defined (ZSTD_MSAN_DONT_POISON_WORKSPACE) + /* To validate that the table re-use logic is sound, and that we don't + * access table space that we haven't cleaned, we re-"poison" the table + * space every time we mark it dirty. + * + * This function however is intended to operate on those dirty tables and + * re-clean them. So when this function is used correctly, we can unpoison + * the memory it operated on. This introduces a blind spot though, since + * if we now try to operate on __actually__ poisoned memory, we will not + * detect that. */ + __msan_unpoison(table, size * sizeof(U32)); +#endif + for (rowNb=0 ; rowNb < nbRows ; rowNb++) { int column; for (column=0; columntableValidEnd - (BYTE*)ws->objectEnd; + assert(__msan_test_shadow(ws->objectEnd, size) == -1); + __msan_poison(ws->objectEnd, size); + } +#endif + assert(ws->tableValidEnd >= ws->objectEnd); assert(ws->tableValidEnd <= ws->allocStart); ws->tableValidEnd = ws->objectEnd; @@ -309,6 +321,18 @@ MEM_STATIC void ZSTD_cwksp_clear_tables(ZSTD_cwksp* ws) { */ MEM_STATIC void ZSTD_cwksp_clear(ZSTD_cwksp* ws) { DEBUGLOG(4, "cwksp: clearing!"); + +#if defined (MEMORY_SANITIZER) && !defined (ZSTD_MSAN_DONT_POISON_WORKSPACE) + /* To validate that the context re-use logic is sound, and that we don't + * access stuff that this compression hasn't initialized, we re-"poison" + * the workspace (or at least the non-static, non-table parts of it) + * every time we start a new compression. */ + { + size_t size = (BYTE*)ws->workspaceEnd - (BYTE*)ws->tableValidEnd; + __msan_poison(ws->tableValidEnd, size); + } +#endif + ws->tableEnd = ws->objectEnd; ws->allocStart = ws->workspaceEnd; ws->allocFailed = 0; From 51d90668ba4f3f23bc09f2b603290691be30f77e Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 16:41:41 -0400 Subject: [PATCH 19/21] Add Assertions to Confirm that Workspace Pointers are Correctly Ordered --- lib/compress/zstd_cwksp.h | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/compress/zstd_cwksp.h b/lib/compress/zstd_cwksp.h index 796b8542..98499b86 100644 --- a/lib/compress/zstd_cwksp.h +++ b/lib/compress/zstd_cwksp.h @@ -147,6 +147,16 @@ typedef struct { MEM_STATIC size_t ZSTD_cwksp_available_space(ZSTD_cwksp* ws); +MEM_STATIC void ZSTD_cwksp_assert_internal_consistency(ZSTD_cwksp* ws) { + (void)ws; + assert(ws->workspace <= ws->objectEnd); + assert(ws->objectEnd <= ws->tableEnd); + assert(ws->objectEnd <= ws->tableValidEnd); + assert(ws->tableEnd <= ws->allocStart); + assert(ws->tableValidEnd <= ws->allocStart); + assert(ws->allocStart <= ws->workspaceEnd); +} + /** * Align must be a power of 2. */ @@ -190,8 +200,10 @@ MEM_STATIC void* ZSTD_cwksp_reserve_internal( alloc = (BYTE *)ws->allocStart - bytes; DEBUGLOG(5, "cwksp: reserving %zd bytes, %zd bytes remaining", bytes, ZSTD_cwksp_available_space(ws) - bytes); + ZSTD_cwksp_assert_internal_consistency(ws); assert(alloc >= bottom); if (alloc < bottom) { + DEBUGLOG(4, "cwksp: alloc failed!"); ws->allocFailed = 1; return NULL; } @@ -231,9 +243,10 @@ MEM_STATIC void* ZSTD_cwksp_reserve_table(ZSTD_cwksp* ws, size_t bytes) { bytes, ZSTD_cwksp_available_space(ws) - bytes); assert((bytes & (sizeof(U32)-1)) == 0); ZSTD_cwksp_internal_advance_phase(ws, phase); + ZSTD_cwksp_assert_internal_consistency(ws); assert(end <= top); if (end > top) { - DEBUGLOG(4, "cwksp: object alloc failed!"); + DEBUGLOG(4, "cwksp: table alloc failed!"); ws->allocFailed = 1; return NULL; } @@ -253,6 +266,7 @@ MEM_STATIC void* ZSTD_cwksp_reserve_object(ZSTD_cwksp* ws, size_t bytes) { bytes, roundedBytes, ZSTD_cwksp_available_space(ws) - roundedBytes); assert(((size_t)start & (sizeof(void*)-1)) == 0); assert((bytes & (sizeof(void*)-1)) == 0); + ZSTD_cwksp_assert_internal_consistency(ws); /* we must be in the first phase, no advance is possible */ if (ws->phase != ZSTD_cwksp_alloc_objects || end > ws->workspaceEnd) { DEBUGLOG(4, "cwksp: object alloc failed!"); @@ -282,6 +296,7 @@ MEM_STATIC void ZSTD_cwksp_mark_tables_dirty(ZSTD_cwksp* ws) { assert(ws->tableValidEnd >= ws->objectEnd); assert(ws->tableValidEnd <= ws->allocStart); ws->tableValidEnd = ws->objectEnd; + ZSTD_cwksp_assert_internal_consistency(ws); } MEM_STATIC void ZSTD_cwksp_mark_tables_clean(ZSTD_cwksp* ws) { @@ -291,6 +306,7 @@ MEM_STATIC void ZSTD_cwksp_mark_tables_clean(ZSTD_cwksp* ws) { if (ws->tableValidEnd < ws->tableEnd) { ws->tableValidEnd = ws->tableEnd; } + ZSTD_cwksp_assert_internal_consistency(ws); } /** @@ -313,6 +329,7 @@ MEM_STATIC void ZSTD_cwksp_clean_tables(ZSTD_cwksp* ws) { MEM_STATIC void ZSTD_cwksp_clear_tables(ZSTD_cwksp* ws) { DEBUGLOG(4, "cwksp: clearing tables!"); ws->tableEnd = ws->objectEnd; + ZSTD_cwksp_assert_internal_consistency(ws); } /** @@ -339,6 +356,7 @@ MEM_STATIC void ZSTD_cwksp_clear(ZSTD_cwksp* ws) { if (ws->phase > ZSTD_cwksp_alloc_buffers) { ws->phase = ZSTD_cwksp_alloc_buffers; } + ZSTD_cwksp_assert_internal_consistency(ws); } /** @@ -356,6 +374,7 @@ MEM_STATIC void ZSTD_cwksp_init(ZSTD_cwksp* ws, void* start, size_t size) { ws->phase = ZSTD_cwksp_alloc_objects; ZSTD_cwksp_clear(ws); ws->workspaceOversizedDuration = 0; + ZSTD_cwksp_assert_internal_consistency(ws); } MEM_STATIC size_t ZSTD_cwksp_create(ZSTD_cwksp* ws, size_t size, ZSTD_customMem customMem) { @@ -369,9 +388,7 @@ MEM_STATIC size_t ZSTD_cwksp_create(ZSTD_cwksp* ws, size_t size, ZSTD_customMem MEM_STATIC void ZSTD_cwksp_free(ZSTD_cwksp* ws, ZSTD_customMem customMem) { DEBUGLOG(4, "cwksp: freeing workspace"); ZSTD_free(ws->workspace, customMem); - ws->workspace = NULL; - ws->workspaceEnd = NULL; - ZSTD_cwksp_clear(ws); + memset(ws, 0, sizeof(ZSTD_cwksp)); } /** From 20c69077d1ce197cb6cf7f1bf8f581fb98607b71 Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Wed, 11 Sep 2019 17:03:09 -0400 Subject: [PATCH 20/21] Shrink Table Valid End During Alloc Alignment / Phase Change --- lib/compress/zstd_cwksp.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/compress/zstd_cwksp.h b/lib/compress/zstd_cwksp.h index 98499b86..39d064c4 100644 --- a/lib/compress/zstd_cwksp.h +++ b/lib/compress/zstd_cwksp.h @@ -184,6 +184,9 @@ MEM_STATIC void ZSTD_cwksp_internal_advance_phase( * by a larger margin than the space that will be consumed. */ /* TODO: cleaner, compiler warning friendly way to do this??? */ ws->allocStart = (BYTE*)ws->allocStart - ((size_t)ws->allocStart & (sizeof(U32)-1)); + if (ws->allocStart < ws->tableValidEnd) { + ws->tableValidEnd = ws->allocStart; + } } ws->phase = phase; } From 72ea79cacd161ba8c2f0cf49217d0df5ff844d5f Mon Sep 17 00:00:00 2001 From: "W. Felix Handte" Date: Mon, 16 Sep 2019 12:08:03 -0400 Subject: [PATCH 21/21] Don't Include `sanitizer/msan_interface.h`, Since Not All Platforms Provide It Instead, explicitly declare the functions we use. --- lib/common/mem.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/common/mem.h b/lib/common/mem.h index 3dc4f428..2b115ddb 100644 --- a/lib/common/mem.h +++ b/lib/common/mem.h @@ -55,7 +55,23 @@ MEM_STATIC void MEM_check(void) { MEM_STATIC_ASSERT((sizeof(size_t)==4) || (size #endif #if defined (MEMORY_SANITIZER) -# include +/* Not all platforms that support msan provide sanitizers/msan_interface.h. + * We therefore declare the functions we need ourselves, rather than trying to + * include the header file... */ + +#include /* intptr_t */ + +/* Make memory region fully initialized (without changing its contents). */ +void __msan_unpoison(const volatile void *a, size_t size); + +/* Make memory region fully uninitialized (without changing its contents). + This is a legacy interface that does not update origin information. Use + __msan_allocated_memory() instead. */ +void __msan_poison(const volatile void *a, size_t size); + +/* Returns the offset of the first (at least partially) poisoned byte in the + memory range, or -1 if the whole range is good. */ +intptr_t __msan_test_shadow(const volatile void *x, size_t size); #endif #if defined (MEMORY_SANITIZER)