From 4d4fd2c55fe87542db8ca30ac354d173a3c29629 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 10:59:35 -0500 Subject: [PATCH 1/7] Overhaul repcode handling logic --- lib/compress/zstd_compress.c | 60 +++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index a1bf866c6..c6abef328 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2498,14 +2498,20 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) ZSTD_Sequence* outSeqs = &zc->seqCollector.seqStart[zc->seqCollector.seqIndex]; size_t i; int repIdx; + U32 rep[ZSTD_REP_NUM]; + U32 shouldUpdateRep; assert(zc->seqCollector.seqIndex + 1 < zc->seqCollector.maxSequences); /* Ensure we have enough space for last literals "sequence" */ assert(zc->seqCollector.maxSequences >= seqStoreSeqSize + 1); + ZSTD_memcpy(rep, repStartValue, ZSTD_REP_NUM * sizeof(U32)); + for (i = 0; i < seqStoreSeqSize; ++i) { + U32 rawOffset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; outSeqs[i].litLength = seqStoreSeqs[i].litLength; outSeqs[i].matchLength = seqStoreSeqs[i].matchLength + MINMATCH; outSeqs[i].rep = 0; + shouldUpdateRep = 1; if (i == seqStore->longLengthPos) { if (seqStore->longLengthID == 1) { @@ -2515,24 +2521,50 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) } } + /* Derive the correct offset from the repcode in seqStore_t */ if (seqStoreSeqs[i].offset <= ZSTD_REP_NUM) { - outSeqs[i].rep = seqStoreSeqs[i].offset; - repIdx = (unsigned int)i - seqStoreSeqs[i].offset; - - if (seqStoreSeqs[i].litLength == 0) { - if (seqStoreSeqs[i].offset < 3) { - --repIdx; - } else { - repIdx = (unsigned int)i - 1; + if (seqStoreSeqs[i].litLength != 0) { + if (seqStoreSeqs[i].offset == 1) { + shouldUpdateRep = 0; + rawOffset = rep[0]; + } else if (seqStoreSeqs[i].offset == 2) { + U32 tmp; + rawOffset = rep[1]; + /* Swap ranks of rep[0] and rep[1] */ + tmp = rep[0]; + rep[0] = rep[1]; + rep[1] = tmp; + shouldUpdateRep = 0; + } else if (seqStoreSeqs[i].offset == 3) { + rawOffset = rep[2]; + } + outSeqs[i].rep = seqStoreSeqs[i].offset; + } else { + /* Litlength == 0 is a special case for repcode handling */ + if (seqStoreSeqs[i].offset == 1) { + U32 tmp; + outSeqs[i].rep = 1; + rawOffset = rep[1]; + /* Swap ranks of rep[0] and rep[1] */ + tmp = rep[0]; + rep[0] = rep[1]; + rep[1] = tmp; + shouldUpdateRep = 0; + } else if (seqStoreSeqs[i].offset == 2) { + outSeqs[i].rep = 2; + rawOffset = rep[2]; + } else if (seqStoreSeqs[i].offset == 3) { + outSeqs[i].rep = 1; + rawOffset = rep[0] - 1; } } - assert(repIdx >= -3); - outSeqs[i].offset = repIdx >= 0 ? outSeqs[repIdx].offset : repStartValue[-repIdx - 1]; - if (outSeqs[i].rep == 3 && outSeqs[i].litLength == 0) { - --outSeqs[i].offset; + } + outSeqs[i].offset = rawOffset; + if (shouldUpdateRep) { + for (int i = ZSTD_REP_NUM - 1; i > 0; i--) { + rep[i] = rep[i - 1]; } - } else { - outSeqs[i].offset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; + rep[0] = outSeqs[i].offset; } literalsRead += outSeqs[i].litLength; } From 1a8af0de73a73ba750d2289ad4290859cce96e1d Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 11:09:01 -0500 Subject: [PATCH 2/7] Improve unit test --- lib/compress/zstd_compress.c | 6 +++--- tests/fuzzer.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index c6abef328..5719c8cbb 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2497,7 +2497,6 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) ZSTD_Sequence* outSeqs = &zc->seqCollector.seqStart[zc->seqCollector.seqIndex]; size_t i; - int repIdx; U32 rep[ZSTD_REP_NUM]; U32 shouldUpdateRep; @@ -2561,8 +2560,9 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) } outSeqs[i].offset = rawOffset; if (shouldUpdateRep) { - for (int i = ZSTD_REP_NUM - 1; i > 0; i--) { - rep[i] = rep[i - 1]; + int j; + for (j = ZSTD_REP_NUM - 1; j > 0; j--) { + rep[j] = rep[j - 1]; } rep[0] = outSeqs[i].offset; } diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 65e442093..3a3fb6a10 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2723,7 +2723,7 @@ static int basicUnitTests(U32 const seed, double compressibility) assert(cctx != NULL); /* Populate src with random data */ - RDG_genBuffer(CNBuffer, srcSize, compressibility, 0., seed); + RDG_genBuffer(CNBuffer, srcSize, compressibility, 0.5, seed); /* Test with block delimiters roundtrip */ seqsSize = ZSTD_generateSequences(cctx, seqs, srcSize, src, srcSize); From 396275068cc64f1afda598723743963cdd380a2e Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 11:57:01 -0500 Subject: [PATCH 3/7] Fix incorrect repcode setting --- lib/compress/zstd_compress.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 5719c8cbb..55ef0fe91 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2522,6 +2522,7 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) /* Derive the correct offset from the repcode in seqStore_t */ if (seqStoreSeqs[i].offset <= ZSTD_REP_NUM) { + outSeqs[i].rep = seqStoreSeqs[i].offset; if (seqStoreSeqs[i].litLength != 0) { if (seqStoreSeqs[i].offset == 1) { shouldUpdateRep = 0; @@ -2537,12 +2538,10 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) } else if (seqStoreSeqs[i].offset == 3) { rawOffset = rep[2]; } - outSeqs[i].rep = seqStoreSeqs[i].offset; } else { /* Litlength == 0 is a special case for repcode handling */ if (seqStoreSeqs[i].offset == 1) { U32 tmp; - outSeqs[i].rep = 1; rawOffset = rep[1]; /* Swap ranks of rep[0] and rep[1] */ tmp = rep[0]; @@ -2550,10 +2549,8 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) rep[1] = tmp; shouldUpdateRep = 0; } else if (seqStoreSeqs[i].offset == 2) { - outSeqs[i].rep = 2; rawOffset = rep[2]; } else if (seqStoreSeqs[i].offset == 3) { - outSeqs[i].rep = 1; rawOffset = rep[0] - 1; } } From 06c7f14066c2afbd479e4bb9c5b461f7e67c2ab7 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 12:22:58 -0500 Subject: [PATCH 4/7] Let block reps persist --- 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 55ef0fe91..f1c0b7d64 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2503,7 +2503,8 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) assert(zc->seqCollector.seqIndex + 1 < zc->seqCollector.maxSequences); /* Ensure we have enough space for last literals "sequence" */ assert(zc->seqCollector.maxSequences >= seqStoreSeqSize + 1); - ZSTD_memcpy(rep, repStartValue, ZSTD_REP_NUM * sizeof(U32)); + ZSTD_memcpy(rep, zc->blockState.prevCBlock->rep, ZSTD_REP_NUM * sizeof(U32)); + DEBUGLOG(2, "%u %u %u", rep[0], rep[1], rep[2]); for (i = 0; i < seqStoreSeqSize; ++i) { U32 rawOffset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; From 674c9b92354bd3152fa58365af1991fdfa353bb0 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 14:37:47 -0500 Subject: [PATCH 5/7] Add in proper block repcode histories --- lib/compress/zstd_compress.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index f1c0b7d64..825fe6b36 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2497,14 +2497,12 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) ZSTD_Sequence* outSeqs = &zc->seqCollector.seqStart[zc->seqCollector.seqIndex]; size_t i; - U32 rep[ZSTD_REP_NUM]; + U32* rep = zc->blockState.prevCBlock->rep; U32 shouldUpdateRep; assert(zc->seqCollector.seqIndex + 1 < zc->seqCollector.maxSequences); /* Ensure we have enough space for last literals "sequence" */ assert(zc->seqCollector.maxSequences >= seqStoreSeqSize + 1); - ZSTD_memcpy(rep, zc->blockState.prevCBlock->rep, ZSTD_REP_NUM * sizeof(U32)); - DEBUGLOG(2, "%u %u %u", rep[0], rep[1], rep[2]); for (i = 0; i < seqStoreSeqSize; ++i) { U32 rawOffset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; @@ -2558,15 +2556,13 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) } outSeqs[i].offset = rawOffset; if (shouldUpdateRep) { - int j; - for (j = ZSTD_REP_NUM - 1; j > 0; j--) { - rep[j] = rep[j - 1]; - } + /* Purge the last repcode, move in new offset */ + rep[2] = rep[1]; + rep[1] = rep[0]; rep[0] = outSeqs[i].offset; } literalsRead += outSeqs[i].litLength; } - /* Insert last literals (if any exist) in the block as a sequence with ml == off == 0. * If there are no last literals, then we'll emit (of: 0, ml: 0, ll: 0), which is a marker * for the block boundary, according to the API. @@ -2577,6 +2573,9 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) outSeqs[i].matchLength = outSeqs[i].offset = outSeqs[i].rep = 0; seqStoreSeqSize++; + zc->blockState.nextCBlock->rep[0] = zc->blockState.prevCBlock->rep[0]; + zc->blockState.nextCBlock->rep[1] = zc->blockState.prevCBlock->rep[1]; + zc->blockState.nextCBlock->rep[2] = zc->blockState.prevCBlock->rep[2]; zc->seqCollector.seqIndex += seqStoreSeqSize; } From be4ac6c5bc2ed08fac33293906881620cce3b970 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Thu, 12 Nov 2020 16:38:23 -0500 Subject: [PATCH 6/7] Use existing repcode update function to implement updates --- lib/compress/zstd_compress.c | 53 +++++++++--------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 825fe6b36..98caf5171 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2497,19 +2497,17 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) ZSTD_Sequence* outSeqs = &zc->seqCollector.seqStart[zc->seqCollector.seqIndex]; size_t i; + repcodes_t updatedRepcodes; U32* rep = zc->blockState.prevCBlock->rep; - U32 shouldUpdateRep; assert(zc->seqCollector.seqIndex + 1 < zc->seqCollector.maxSequences); /* Ensure we have enough space for last literals "sequence" */ assert(zc->seqCollector.maxSequences >= seqStoreSeqSize + 1); - for (i = 0; i < seqStoreSeqSize; ++i) { U32 rawOffset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; outSeqs[i].litLength = seqStoreSeqs[i].litLength; outSeqs[i].matchLength = seqStoreSeqs[i].matchLength + MINMATCH; outSeqs[i].rep = 0; - shouldUpdateRep = 1; if (i == seqStore->longLengthPos) { if (seqStore->longLengthID == 1) { @@ -2519,50 +2517,26 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) } } - /* Derive the correct offset from the repcode in seqStore_t */ if (seqStoreSeqs[i].offset <= ZSTD_REP_NUM) { + /* Derive the correct offset corresponding to a repcode */ outSeqs[i].rep = seqStoreSeqs[i].offset; - if (seqStoreSeqs[i].litLength != 0) { - if (seqStoreSeqs[i].offset == 1) { - shouldUpdateRep = 0; - rawOffset = rep[0]; - } else if (seqStoreSeqs[i].offset == 2) { - U32 tmp; - rawOffset = rep[1]; - /* Swap ranks of rep[0] and rep[1] */ - tmp = rep[0]; - rep[0] = rep[1]; - rep[1] = tmp; - shouldUpdateRep = 0; - } else if (seqStoreSeqs[i].offset == 3) { - rawOffset = rep[2]; - } + if (outSeqs[i].litLength != 0) { + rawOffset = rep[outSeqs[i].rep - 1]; } else { - /* Litlength == 0 is a special case for repcode handling */ - if (seqStoreSeqs[i].offset == 1) { - U32 tmp; - rawOffset = rep[1]; - /* Swap ranks of rep[0] and rep[1] */ - tmp = rep[0]; - rep[0] = rep[1]; - rep[1] = tmp; - shouldUpdateRep = 0; - } else if (seqStoreSeqs[i].offset == 2) { - rawOffset = rep[2]; - } else if (seqStoreSeqs[i].offset == 3) { + if (outSeqs[i].rep == 3) { rawOffset = rep[0] - 1; + } else { + rawOffset = rep[outSeqs[i].rep]; } } } outSeqs[i].offset = rawOffset; - if (shouldUpdateRep) { - /* Purge the last repcode, move in new offset */ - rep[2] = rep[1]; - rep[1] = rep[0]; - rep[0] = outSeqs[i].offset; - } + updatedRepcodes = ZSTD_updateRep(rep, seqStoreSeqs[i].offset - 1, seqStoreSeqs[i].litLength == 0); + ZSTD_memcpy(rep, updatedRepcodes.rep, sizeof(repcodes_t)); + literalsRead += outSeqs[i].litLength; } + /* Insert last literals (if any exist) in the block as a sequence with ml == off == 0. * If there are no last literals, then we'll emit (of: 0, ml: 0, ll: 0), which is a marker * for the block boundary, according to the API. @@ -2573,9 +2547,8 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) outSeqs[i].matchLength = outSeqs[i].offset = outSeqs[i].rep = 0; seqStoreSeqSize++; - zc->blockState.nextCBlock->rep[0] = zc->blockState.prevCBlock->rep[0]; - zc->blockState.nextCBlock->rep[1] = zc->blockState.prevCBlock->rep[1]; - zc->blockState.nextCBlock->rep[2] = zc->blockState.prevCBlock->rep[2]; + ZSTD_memcpy(zc->blockState.nextCBlock->rep, zc->blockState.prevCBlock->rep, + sizeof(zc->blockState.nextCBlock->rep)); zc->seqCollector.seqIndex += seqStoreSeqSize; } From 9d936d61d212e950f5134d09d3bcae164c0c3b31 Mon Sep 17 00:00:00 2001 From: senhuang42 Date: Fri, 13 Nov 2020 09:41:44 -0500 Subject: [PATCH 7/7] Reduce number of memcpy() calls --- lib/compress/zstd_compress.c | 20 +++++++++----------- tests/fuzzer.c | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 98caf5171..c21a8c982 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2498,11 +2498,11 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) ZSTD_Sequence* outSeqs = &zc->seqCollector.seqStart[zc->seqCollector.seqIndex]; size_t i; repcodes_t updatedRepcodes; - U32* rep = zc->blockState.prevCBlock->rep; assert(zc->seqCollector.seqIndex + 1 < zc->seqCollector.maxSequences); /* Ensure we have enough space for last literals "sequence" */ assert(zc->seqCollector.maxSequences >= seqStoreSeqSize + 1); + ZSTD_memcpy(updatedRepcodes.rep, zc->blockState.prevCBlock->rep, sizeof(repcodes_t)); for (i = 0; i < seqStoreSeqSize; ++i) { U32 rawOffset = seqStoreSeqs[i].offset - ZSTD_REP_NUM; outSeqs[i].litLength = seqStoreSeqs[i].litLength; @@ -2521,22 +2521,23 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) /* Derive the correct offset corresponding to a repcode */ outSeqs[i].rep = seqStoreSeqs[i].offset; if (outSeqs[i].litLength != 0) { - rawOffset = rep[outSeqs[i].rep - 1]; + rawOffset = updatedRepcodes.rep[outSeqs[i].rep - 1]; } else { if (outSeqs[i].rep == 3) { - rawOffset = rep[0] - 1; + rawOffset = updatedRepcodes.rep[0] - 1; } else { - rawOffset = rep[outSeqs[i].rep]; + rawOffset = updatedRepcodes.rep[outSeqs[i].rep]; } } } outSeqs[i].offset = rawOffset; - updatedRepcodes = ZSTD_updateRep(rep, seqStoreSeqs[i].offset - 1, seqStoreSeqs[i].litLength == 0); - ZSTD_memcpy(rep, updatedRepcodes.rep, sizeof(repcodes_t)); - + /* seqStoreSeqs[i].offset == offCode+1, and ZSTD_updateRep() expects offCode + so we provide seqStoreSeqs[i].offset - 1 */ + updatedRepcodes = ZSTD_updateRep(updatedRepcodes.rep, + seqStoreSeqs[i].offset - 1, + seqStoreSeqs[i].litLength == 0); literalsRead += outSeqs[i].litLength; } - /* Insert last literals (if any exist) in the block as a sequence with ml == off == 0. * If there are no last literals, then we'll emit (of: 0, ml: 0, ll: 0), which is a marker * for the block boundary, according to the API. @@ -2546,9 +2547,6 @@ static void ZSTD_copyBlockSequences(ZSTD_CCtx* zc) outSeqs[i].litLength = (U32)lastLLSize; outSeqs[i].matchLength = outSeqs[i].offset = outSeqs[i].rep = 0; seqStoreSeqSize++; - - ZSTD_memcpy(zc->blockState.nextCBlock->rep, zc->blockState.prevCBlock->rep, - sizeof(zc->blockState.nextCBlock->rep)); zc->seqCollector.seqIndex += seqStoreSeqSize; } diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 3a3fb6a10..2afd10935 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -2721,7 +2721,7 @@ static int basicUnitTests(U32 const seed, double compressibility) if (seqs == NULL) goto _output_error; assert(cctx != NULL); - + ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, 19); /* Populate src with random data */ RDG_genBuffer(CNBuffer, srcSize, compressibility, 0.5, seed);