From fdb2780c47727eb8ec0fb354dfc4c6730bbd67d1 Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Sat, 1 Feb 2020 11:56:31 -0800 Subject: [PATCH 1/6] Move rank table into HUF_buildCTable_wksp() --- lib/compress/huf_compress.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c index 5cab31d04..f81a2f386 100644 --- a/lib/compress/huf_compress.c +++ b/lib/compress/huf_compress.c @@ -295,12 +295,11 @@ typedef struct { U32 current; } rankPos; -static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValue) +static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValue, rankPos* rank) { - rankPos rank[32]; U32 n; - memset(rank, 0, sizeof(rank)); + memset(rank, 0, sizeof(rankPos)*32); for (n=0; n<=maxSymbolValue; n++) { U32 r = BIT_highbit32(count[n] + 1); rank[r].base ++; @@ -335,6 +334,7 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo int lowS, lowN; U16 nodeNb = STARTNODE; U32 nodeRoot; + rankPos rank[32]; /* safety checks */ if (((size_t)workSpace & 3) != 0) return ERROR(GENERIC); /* must be aligned on 4-bytes boundaries */ @@ -344,7 +344,7 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo memset(huffNode0, 0, sizeof(huffNodeTable)); /* sort, decreasing order */ - HUF_sort(huffNode, count, maxSymbolValue); + HUF_sort(huffNode, count, maxSymbolValue, rank); /* init for parents */ nonNullRank = maxSymbolValue; From a084d959bdba329b4fce512e7ca1b8c8208cc371 Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Sat, 1 Feb 2020 12:41:05 -0800 Subject: [PATCH 2/6] WIP: Increased wksp size, but it's segfaulting --- lib/common/huf.h | 2 +- lib/compress/huf_compress.c | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/common/huf.h b/lib/common/huf.h index 0d27ccdba..53b1ad693 100644 --- a/lib/common/huf.h +++ b/lib/common/huf.h @@ -90,7 +90,7 @@ HUF_PUBLIC_API size_t HUF_compress2 (void* dst, size_t dstCapacity, /** HUF_compress4X_wksp() : * Same as HUF_compress2(), but uses externally allocated `workSpace`. * `workspace` must have minimum alignment of 4, and be at least as large as HUF_WORKSPACE_SIZE */ -#define HUF_WORKSPACE_SIZE (6 << 10) +#define HUF_WORKSPACE_SIZE ((6 << 10) + 256) #define HUF_WORKSPACE_SIZE_U32 (HUF_WORKSPACE_SIZE / sizeof(U32)) HUF_PUBLIC_API size_t HUF_compress4X_wksp (void* dst, size_t dstCapacity, const void* src, size_t srcSize, diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c index f81a2f386..f0b210725 100644 --- a/lib/compress/huf_compress.c +++ b/lib/compress/huf_compress.c @@ -299,7 +299,7 @@ static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValu { U32 n; - memset(rank, 0, sizeof(rankPos)*32); + memset(rank, 0, sizeof(rankPos) * 32); for (n=0; n<=maxSymbolValue; n++) { U32 r = BIT_highbit32(count[n] + 1); rank[r].base ++; @@ -334,11 +334,11 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo int lowS, lowN; U16 nodeNb = STARTNODE; U32 nodeRoot; - rankPos rank[32]; - + rankPos* rank = (rankPos *)(huffNode + 1); + DEBUGLOG(3, "workspace %p huffnode0 %p huffnode %p rank %p workspaceend %p", workSpace, huffNode0, huffNode, rank, ((BYTE*)workSpace) + wkspSize); /* safety checks */ if (((size_t)workSpace & 3) != 0) return ERROR(GENERIC); /* must be aligned on 4-bytes boundaries */ - if (wkspSize < sizeof(huffNodeTable)) return ERROR(workSpace_tooSmall); + if (wkspSize < sizeof(huffNodeTable) + sizeof(rankPos) * 32) return ERROR(workSpace_tooSmall); if (maxNbBits == 0) maxNbBits = HUF_TABLELOG_DEFAULT; if (maxSymbolValue > HUF_SYMBOLVALUE_MAX) return ERROR(maxSymbolValue_tooLarge); memset(huffNode0, 0, sizeof(huffNodeTable)); @@ -403,8 +403,9 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo */ size_t HUF_buildCTable (HUF_CElt* tree, const unsigned* count, unsigned maxSymbolValue, unsigned maxNbBits) { - huffNodeTable nodeTable; - return HUF_buildCTable_wksp(tree, count, maxSymbolValue, maxNbBits, nodeTable, sizeof(nodeTable)); + U32 workspace[HUF_WORKSPACE_SIZE_U32]; + DEBUGLOG(3, "!!!"); + return HUF_buildCTable_wksp(tree, count, maxSymbolValue, maxNbBits, workspace, sizeof(workspace)); } size_t HUF_estimateCompressedSize(const HUF_CElt* CTable, const unsigned* count, unsigned maxSymbolValue) From fe8402b5223c8b716e8c0b35884e317ae4478cef Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Sat, 1 Feb 2020 15:34:51 -0800 Subject: [PATCH 3/6] WIP: Still getting an error --- lib/compress/huf_compress.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c index f0b210725..179a68f45 100644 --- a/lib/compress/huf_compress.c +++ b/lib/compress/huf_compress.c @@ -334,8 +334,8 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo int lowS, lowN; U16 nodeNb = STARTNODE; U32 nodeRoot; - rankPos* rank = (rankPos *)(huffNode + 1); - DEBUGLOG(3, "workspace %p huffnode0 %p huffnode %p rank %p workspaceend %p", workSpace, huffNode0, huffNode, rank, ((BYTE*)workSpace) + wkspSize); + rankPos* rank = (rankPos *)(huffNode0 + HUF_CTABLE_WORKSPACE_SIZE / sizeof(nodeElt*)); + /* safety checks */ if (((size_t)workSpace & 3) != 0) return ERROR(GENERIC); /* must be aligned on 4-bytes boundaries */ if (wkspSize < sizeof(huffNodeTable) + sizeof(rankPos) * 32) return ERROR(workSpace_tooSmall); @@ -404,7 +404,6 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo size_t HUF_buildCTable (HUF_CElt* tree, const unsigned* count, unsigned maxSymbolValue, unsigned maxNbBits) { U32 workspace[HUF_WORKSPACE_SIZE_U32]; - DEBUGLOG(3, "!!!"); return HUF_buildCTable_wksp(tree, count, maxSymbolValue, maxNbBits, workspace, sizeof(workspace)); } @@ -672,7 +671,7 @@ HUF_compress_internal (void* dst, size_t dstSize, huffLog = HUF_optimalTableLog(huffLog, srcSize, maxSymbolValue); { size_t const maxBits = HUF_buildCTable_wksp(table->CTable, table->count, maxSymbolValue, huffLog, - table->nodeTable, sizeof(table->nodeTable)); + table->nodeTable, ((BYTE*)workSpace) + wkspSize - (BYTE*)table->nodeTable); CHECK_F(maxBits); huffLog = (U32)maxBits; /* Zero unused symbols in CTable, so we can check it for validity */ From cc7c29595d7591eb901973bf9c5fc6c854d086ca Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Sat, 1 Feb 2020 18:26:14 -0800 Subject: [PATCH 4/6] Fixed tests to use correct workspace size --- tests/decodecorpus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/decodecorpus.c b/tests/decodecorpus.c index af961bfd0..a46fc24d8 100644 --- a/tests/decodecorpus.c +++ b/tests/decodecorpus.c @@ -185,7 +185,7 @@ BYTE SEQUENCE_LLCODE[ZSTD_BLOCKSIZE_MAX]; BYTE SEQUENCE_MLCODE[ZSTD_BLOCKSIZE_MAX]; BYTE SEQUENCE_OFCODE[ZSTD_BLOCKSIZE_MAX]; -unsigned WKSP[1024]; +unsigned WKSP[HUF_WORKSPACE_SIZE_U32]; typedef struct { size_t contentSize; /* 0 means unknown (unless contentSize == windowSize == 0) */ From 53d76dc20f57ee67d247be6d86d5372976eb84dd Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Thu, 27 Feb 2020 22:02:44 +0530 Subject: [PATCH 5/6] Remove magic constant and made other changes addressing the comments --- lib/compress/huf_compress.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c index 179a68f45..8f9648421 100644 --- a/lib/compress/huf_compress.c +++ b/lib/compress/huf_compress.c @@ -289,28 +289,34 @@ static U32 HUF_setMaxHeight(nodeElt* huffNode, U32 lastNonNull, U32 maxNbBits) return maxNbBits; } - typedef struct { U32 base; U32 current; } rankPos; -static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValue, rankPos* rank) +typedef nodeElt huffNodeTable[HUF_CTABLE_WORKSPACE_SIZE_U32]; + +typedef struct { + rankPos rankPosition[32]; + huffNodeTable huffNodeTbl; +} HUF_buildCTable_wksp_tables; + +static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValue, rankPos* rankPosition) { U32 n; - memset(rank, 0, sizeof(rankPos) * 32); + memset(rankPosition, 0, sizeof(*rankPosition)); for (n=0; n<=maxSymbolValue; n++) { U32 r = BIT_highbit32(count[n] + 1); - rank[r].base ++; + rankPosition[r].base ++; } - for (n=30; n>0; n--) rank[n-1].base += rank[n].base; - for (n=0; n<32; n++) rank[n].current = rank[n].base; + for (n=30; n>0; n--) rankPosition[n-1].base += rankPosition[n].base; + for (n=0; n<32; n++) rankPosition[n].current = rankPosition[n].base; for (n=0; n<=maxSymbolValue; n++) { U32 const c = count[n]; U32 const r = BIT_highbit32(c+1) + 1; - U32 pos = rank[r].current++; - while ((pos > rank[r].base) && (c > huffNode[pos-1].count)) { + U32 pos = rankPosition[r].current++; + while ((pos > rankPosition[r].base) && (c > huffNode[pos-1].count)) { huffNode[pos] = huffNode[pos-1]; pos--; } @@ -325,26 +331,28 @@ static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValu * `workSpace` must be aligned on 4-bytes boundaries, and be at least as large as a table of HUF_CTABLE_WORKSPACE_SIZE_U32 unsigned. */ #define STARTNODE (HUF_SYMBOLVALUE_MAX+1) -typedef nodeElt huffNodeTable[HUF_CTABLE_WORKSPACE_SIZE_U32]; + size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbolValue, U32 maxNbBits, void* workSpace, size_t wkspSize) { - nodeElt* const huffNode0 = (nodeElt*)workSpace; + HUF_buildCTable_wksp_tables* const wksp_tables = (HUF_buildCTable_wksp_tables*)workSpace; + nodeElt* const huffNode0 = wksp_tables->huffNodeTbl; nodeElt* const huffNode = huffNode0+1; U32 n, nonNullRank; int lowS, lowN; U16 nodeNb = STARTNODE; U32 nodeRoot; - rankPos* rank = (rankPos *)(huffNode0 + HUF_CTABLE_WORKSPACE_SIZE / sizeof(nodeElt*)); /* safety checks */ if (((size_t)workSpace & 3) != 0) return ERROR(GENERIC); /* must be aligned on 4-bytes boundaries */ - if (wkspSize < sizeof(huffNodeTable) + sizeof(rankPos) * 32) return ERROR(workSpace_tooSmall); + if (wkspSize < sizeof(HUF_buildCTable_wksp_tables)) + return ERROR(workSpace_tooSmall); if (maxNbBits == 0) maxNbBits = HUF_TABLELOG_DEFAULT; - if (maxSymbolValue > HUF_SYMBOLVALUE_MAX) return ERROR(maxSymbolValue_tooLarge); + if (maxSymbolValue > HUF_SYMBOLVALUE_MAX) + return ERROR(maxSymbolValue_tooLarge); memset(huffNode0, 0, sizeof(huffNodeTable)); /* sort, decreasing order */ - HUF_sort(huffNode, count, maxSymbolValue, rank); + HUF_sort(huffNode, count, maxSymbolValue, wksp_tables->rankPosition); /* init for parents */ nonNullRank = maxSymbolValue; From 0adfc8dfcef352e098446c59a14950bdf9b2028d Mon Sep 17 00:00:00 2001 From: Meghna Malhotra Date: Tue, 31 Mar 2020 22:46:49 -0700 Subject: [PATCH 6/6] Fix broken CI; make changes in response to the comments --- lib/compress/huf_compress.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c index 8f9648421..28762800c 100644 --- a/lib/compress/huf_compress.c +++ b/lib/compress/huf_compress.c @@ -296,16 +296,18 @@ typedef struct { typedef nodeElt huffNodeTable[HUF_CTABLE_WORKSPACE_SIZE_U32]; +#define RANK_POSITION_TABLE_SIZE 32 + typedef struct { - rankPos rankPosition[32]; huffNodeTable huffNodeTbl; + rankPos rankPosition[RANK_POSITION_TABLE_SIZE]; } HUF_buildCTable_wksp_tables; static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValue, rankPos* rankPosition) { U32 n; - memset(rankPosition, 0, sizeof(*rankPosition)); + memset(rankPosition, 0, sizeof(*rankPosition) * RANK_POSITION_TABLE_SIZE); for (n=0; n<=maxSymbolValue; n++) { U32 r = BIT_highbit32(count[n] + 1); rankPosition[r].base ++; @@ -328,7 +330,7 @@ static void HUF_sort(nodeElt* huffNode, const unsigned* count, U32 maxSymbolValu /** HUF_buildCTable_wksp() : * Same as HUF_buildCTable(), but using externally allocated scratch buffer. - * `workSpace` must be aligned on 4-bytes boundaries, and be at least as large as a table of HUF_CTABLE_WORKSPACE_SIZE_U32 unsigned. + * `workSpace` must be aligned on 4-bytes boundaries, and be at least as large as sizeof(HUF_buildCTable_wksp_tables). */ #define STARTNODE (HUF_SYMBOLVALUE_MAX+1) @@ -411,8 +413,8 @@ size_t HUF_buildCTable_wksp (HUF_CElt* tree, const unsigned* count, U32 maxSymbo */ size_t HUF_buildCTable (HUF_CElt* tree, const unsigned* count, unsigned maxSymbolValue, unsigned maxNbBits) { - U32 workspace[HUF_WORKSPACE_SIZE_U32]; - return HUF_buildCTable_wksp(tree, count, maxSymbolValue, maxNbBits, workspace, sizeof(workspace)); + HUF_buildCTable_wksp_tables workspace; + return HUF_buildCTable_wksp(tree, count, maxSymbolValue, maxNbBits, &workspace, sizeof(workspace)); } size_t HUF_estimateCompressedSize(const HUF_CElt* CTable, const unsigned* count, unsigned maxSymbolValue) @@ -619,7 +621,7 @@ static size_t HUF_compressCTable_internal( typedef struct { unsigned count[HUF_SYMBOLVALUE_MAX + 1]; HUF_CElt CTable[HUF_SYMBOLVALUE_MAX + 1]; - huffNodeTable nodeTable; + HUF_buildCTable_wksp_tables buildCTable_wksp; } HUF_compress_tables_t; /* HUF_compress_internal() : @@ -638,6 +640,8 @@ HUF_compress_internal (void* dst, size_t dstSize, BYTE* const oend = ostart + dstSize; BYTE* op = ostart; + HUF_STATIC_ASSERT(sizeof(*table) <= HUF_WORKSPACE_SIZE); + /* checks & inits */ if (((size_t)workSpace & 3) != 0) return ERROR(GENERIC); /* must be aligned on 4-bytes boundaries */ if (wkspSize < HUF_WORKSPACE_SIZE) return ERROR(workSpace_tooSmall); @@ -679,7 +683,7 @@ HUF_compress_internal (void* dst, size_t dstSize, huffLog = HUF_optimalTableLog(huffLog, srcSize, maxSymbolValue); { size_t const maxBits = HUF_buildCTable_wksp(table->CTable, table->count, maxSymbolValue, huffLog, - table->nodeTable, ((BYTE*)workSpace) + wkspSize - (BYTE*)table->nodeTable); + &table->buildCTable_wksp, sizeof(table->buildCTable_wksp)); CHECK_F(maxBits); huffLog = (U32)maxBits; /* Zero unused symbols in CTable, so we can check it for validity */