Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Put windowFrac in the CCtxParams, not the CParams #4305

Draft
wants to merge 36 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
22572d6
Separate Public and Private CParam Structs
felixhandte Jan 29, 2025
768a55c
Also Separate Public and Private Params Structs
felixhandte Jan 29, 2025
aa76d3f
Add a `windowFrac` Field to `ZSTD_CParams`
felixhandte Jan 29, 2025
139b725
Add Public `ZSTD_c_windowFrac` CCtxParam
felixhandte Jan 29, 2025
7e695b8
Fix Fuzzer Tests
felixhandte Jan 30, 2025
4dbc1a1
Implement Support for Fractional Window Sizes
felixhandte Jan 29, 2025
3e308e2
Easy: Rename Arg to `ZSTD_cycleLog`
felixhandte Jan 30, 2025
a8da454
Trivial `fuzzer.c` Additions
felixhandte Jan 30, 2025
301a34d
Macro Guard to Control Picking Fractional Window Sizes
felixhandte Jan 30, 2025
a2f3b98
A Little More Bounds-Checking
felixhandte Feb 3, 2025
0561a50
Add Enablement Flag to Some GitHub Workflows
felixhandte Feb 3, 2025
fe80106
Only Apply `windowFrac` when `windowLog` is Also Set
felixhandte Feb 3, 2025
abc3d6f
Avoid Illegal WL and WF Combos
felixhandte Feb 5, 2025
78996a6
Correct Corner-Case Bugs in Window Sizing
felixhandte Feb 6, 2025
9848c21
Exhaustive Testing of Window Sizing Combinations
felixhandte Feb 5, 2025
705a5b2
Add WindowFrac Param to Fuzz Tests
felixhandte Feb 6, 2025
d720558
Add Advanced Parameter to CLI to Set Window Fraction
felixhandte Feb 6, 2025
8b12bd6
Add Man Page Documentation
felixhandte Feb 6, 2025
2af69f9
Add CLI Test for Fractional Window Size Param
felixhandte Feb 6, 2025
72c38c9
Fix Various Contbuild Warnings
felixhandte Feb 7, 2025
80bf084
Fix Hash-Table Sizing of Small Inputs
felixhandte Feb 18, 2025
2404101
ZSTD_MatchState_t Stores CCtxParams Pointer, not CParams
felixhandte Feb 7, 2025
885b21e
Move `useRowMatchFinder` into CCtxParams in CDict
felixhandte Feb 18, 2025
e423daa
Move `compressionLevel` into CCtxParams in CDict
felixhandte Feb 18, 2025
28ce1ee
Convert `ZSTD_adjustCParams_internal()` to Operate on CCtxParams
felixhandte Feb 19, 2025
8832b95
Convert `ZSTD_estimateCCtxSize_usingCCtxParams_internal()` to Operate…
felixhandte Feb 19, 2025
4494d0a
Convert Window Size Helper Functions to take CCtxParams
felixhandte Feb 19, 2025
90c0e0a
Split `ZSTD_checkCParams_internal()` into Two Funcs, One of Which Ope…
felixhandte Feb 20, 2025
004f344
Convert `ZSTD_clampCParams()` to Operate on CCtxParams
felixhandte Feb 20, 2025
c595077
Convert `ZSTD_ldm_adjustParameters()` to take CCtxParams
felixhandte Feb 20, 2025
4fad13e
Move `windowFrac` Field out of CParams into CCtxParams
felixhandte Feb 20, 2025
196ac9d
Unify CParams / Params Structs
felixhandte Feb 20, 2025
cbbf3b2
Further Transport CCtxParams Rather than CParams in Various Param Res…
felixhandte Feb 24, 2025
af60a97
Init CCtxParams from (CParam-like) CCtxParams in Entry-Points
felixhandte Feb 24, 2025
67a2b3c
Fix Transporting FParams in `ZSTD_compressBegin_usingCDict_internal()`
felixhandte Feb 24, 2025
b5fabd7
Fix Build
felixhandte Feb 24, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions .github/workflows/dev-long-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # tag=v4.1.1
- name: make test
run: make test
run: MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make test

# lasts ~26mn
make-test-macos:
Expand All @@ -51,7 +51,7 @@ jobs:
sudo apt-get -qqq update
make libc6install
make clean
CFLAGS="-m32 -O2" make -j test V=1
CFLAGS="-m32 -O2" MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make -j test V=1

no-intrinsics-fuzztest:
runs-on: ubuntu-latest
Expand All @@ -72,7 +72,7 @@ jobs:
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # tag=v4.1.1
- name: ub + address sanitizer on zstreamtest
run: CC=clang make uasan-test-zstream
run: CC=clang MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make uasan-test-zstream

# lasts ~15mn
tsan-fuzztest:
Expand All @@ -91,7 +91,7 @@ jobs:
run: |
sudo apt-get -qqq update
make libc6install
CC=clang make -C tests test-zstream32 FUZZER_FLAGS="--big-tests"
CC=clang MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make -C tests test-zstream32 FUZZER_FLAGS="--big-tests"

# lasts ~23mn
gcc-8-asan-ubsan-testzstd:
Expand Down Expand Up @@ -121,7 +121,7 @@ jobs:
run: |
sudo apt-get -qqq update
make libc6install
make -j uasan-test-zstd32 V=1
MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make -j uasan-test-zstd32 V=1

# Note : external libraries must be turned off when using MSAN tests,
# because they are not msan-instrumented,
Expand All @@ -144,7 +144,7 @@ jobs:
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # tag=v4.1.1
- name: clang + ASan + UBSan + Fuzz Test
run: CC=clang FUZZER_FLAGS="--long-tests" make clean uasan-fuzztest
run: CC=clang FUZZER_FLAGS="--long-tests" MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" make clean uasan-fuzztest

gcc-asan-ubsan-fuzz32:
runs-on: ubuntu-latest
Expand All @@ -154,7 +154,7 @@ jobs:
run: |
sudo apt-get -qqq update
make libc6install
CFLAGS="-O3 -m32" FUZZER_FLAGS="--long-tests" make uasan-fuzztest
CFLAGS="-O3 -m32" MOREFLAGS="-DZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES=1" FUZZER_FLAGS="--long-tests" make uasan-fuzztest

clang-asan-fuzz32:
runs-on: ubuntu-latest
Expand Down
726 changes: 432 additions & 294 deletions lib/compress/zstd_compress.c

Large diffs are not rendered by default.

125 changes: 112 additions & 13 deletions lib/compress/zstd_compress_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,9 @@ struct ZSTD_MatchState_t {
*/
optState_t opt; /* optimal parser state */
const ZSTD_MatchState_t* dictMatchState;
ZSTD_compressionParameters cParams;

const ZSTD_CCtx_params* cctxParams;

const RawSeqStore_t* ldmSeqStore;

/* Controls prefetching in some dictMatchState matchfinders.
Expand Down Expand Up @@ -348,8 +350,9 @@ typedef struct {
U32 hashLog; /* Log size of hashTable */
U32 bucketSizeLog; /* Log bucket size for collision resolution, at most 8 */
U32 minMatchLength; /* Minimum match length */
U32 hashRateLog; /* Log number of entries to skip */
U32 hashRateLog; /* Log number of entries to skip */
U32 windowLog; /* Window log for the LDM */
U32 windowFrac; /* Window log for the LDM */
} ldmParams_t;

typedef struct {
Expand All @@ -362,6 +365,7 @@ typedef struct {
struct ZSTD_CCtx_params_s {
ZSTD_format_e format;
ZSTD_compressionParameters cParams;
unsigned windowFrac; /* Additional CParam controlling sub-power-of-two window sizing. */
ZSTD_frameParameters fParams;

int compressionLevel;
Expand Down Expand Up @@ -1077,6 +1081,88 @@ MEM_STATIC ZSTD_dictMode_e ZSTD_matchState_dictMode(const ZSTD_MatchState_t *ms)
ZSTD_noDict;
}

/**
* Fractional window sizes can always be picked by the user explicitly
* setting ZSTD_c_windowFrac. This macro controls whether, when Zstd is
* picking a window size itself, it is allowed to pick a non-power-of-two
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

* window size.
*
* For now, this defaults to false.
*/
#ifndef ZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES
#define ZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES 0
#endif

/**
* Return the window size described by the windowLog and windowFrac in the
* provided CParams.
*/
MEM_STATIC U32 ZSTD_windowSize(const ZSTD_CCtx_params* params) {
return (U32)(((8ull + params->windowFrac) << params->cParams.windowLog) >> 3);
}
MEM_STATIC U32 ZSTD_windowSizeLDM(const ldmParams_t* ldmParams) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: it seems this function is only useful within zstd_ldm.c.
In which case, is there any benefit in sharing it more widely within zstd_compress_internal.h ?

return (U32)(((8ull + ldmParams->windowFrac) << ldmParams->windowLog) >> 3);
}

/**
* Checks that the selected windowSize satisfies the inequality
* `srcSize <= windowSize <= srcSize * margin`, in the range where the window
* isn't pressed up against one of the hard bounds, where `margin` is either
* 1.125 or 2, depending on whether we're allowed to pick fractional window
* sizes.
*/
MEM_STATIC int ZSTD_windowLogAndFracAreMinimal(const ZSTD_CCtx_params* params, const U32 srcSize) {
const U32 lowerBound = MIN(srcSize, 1u << ZSTD_WINDOWLOG_MAX);
#if ZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES
const U32 upperBound = MAX(lowerBound + (lowerBound >> 3), 1u << ZSTD_WINDOWLOG_ABSOLUTEMIN);
#else
const U32 upperBound = MAX(2 * lowerBound - 1, 1u << ZSTD_WINDOWLOG_ABSOLUTEMIN);
#endif
const U32 windowSize = ZSTD_windowSize(params);
if (windowSize < lowerBound) {
return 0;
}
if (windowSize > upperBound) {
return 0;
}
return 1;
}

/**
* Calculates the minimum legal window log and fraction that contain the
* provided source size.
*/
MEM_STATIC void ZSTD_setMinimalWindowLogAndFrac(ZSTD_CCtx_params* params, const U32 srcSize, const U32 minWindowLog) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here,
ZSTD_setMinimalWindowLogAndFrac() seems only useful within zstd_compress.c.
What benefit is there to share it more widely within zstd_compress_internal.h ?

Note that ZSTD_windowLogAndFracAreMinimal() above seems to be a dependency of this function.

const U32 minSize = 1u << minWindowLog;
#if ZSTD_WINDOW_ALLOW_PICKING_FRACTIONAL_SIZES
if (srcSize < minSize) {
params->cParams.windowLog = minWindowLog;
params->windowFrac = 0;
} else {
const U32 srcSizeMinusOne = srcSize - 1;
params->cParams.windowLog = ZSTD_highbit32(srcSizeMinusOne);
params->windowFrac = ((srcSizeMinusOne >> (params->cParams.windowLog - 3)) & 7) + 1;
if (params->windowFrac == 8) {
params->windowFrac = 0;
params->cParams.windowLog++;
}
}
#else
if (srcSize < minSize) {
params->cParams.windowLog = minWindowLog;
params->windowFrac = 0;
} else {
params->cParams.windowLog = ZSTD_highbit32(srcSize - 1) + 1;
params->windowFrac = 0;
}
#endif
if (params->cParams.windowLog + !!params->windowFrac > ZSTD_WINDOWLOG_MAX) {
params->cParams.windowLog = ZSTD_WINDOWLOG_MAX;
params->windowFrac = 0;
}
assert(ZSTD_windowLogAndFracAreMinimal(params, srcSize));
}

/* Defining this macro to non-zero tells zstd to run the overflow correction
* code much more frequently. This is very inefficient, and should only be
* used for tests and fuzzers.
Expand All @@ -1100,10 +1186,12 @@ MEM_STATIC U32 ZSTD_window_canOverflowCorrect(ZSTD_window_t const window,
U32 loadedDictEnd,
void const* src)
{
/* overflow correction only handles power-of-two index moves. */
U32 const roundedMaxDist = 1u << (ZSTD_highbit32(maxDist - 1) + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is this change fixing some kind of potential bug ?

U32 const cycleSize = 1u << cycleLog;
U32 const curr = (U32)((BYTE const*)src - window.base);
U32 const minIndexToOverflowCorrect = cycleSize
+ MAX(maxDist, cycleSize)
+ MAX(roundedMaxDist, cycleSize)
+ ZSTD_WINDOW_START_INDEX;

/* Adjust the min index to backoff the overflow correction frequency,
Expand Down Expand Up @@ -1178,23 +1266,29 @@ U32 ZSTD_window_correctOverflow(ZSTD_window_t* window, U32 cycleLog,
* 3. (cctx->lowLimit + 1<<windowLog) < 1<<32:
* windowLog <= 31 ==> 3<<29 + 1<<windowLog < 7<<29 < 1<<32.
*/
U32 const roundedMaxDist = 1u << (ZSTD_highbit32(maxDist - 1) + 1);
U32 const cycleSize = 1u << cycleLog;
U32 const cycleMask = cycleSize - 1;
U32 const curr = (U32)((BYTE const*)src - window->base);
U32 const currentCycle = curr & cycleMask;
/* Ensure newCurrent - maxDist >= ZSTD_WINDOW_START_INDEX. */
/* Ensure newCurrent - roundedMaxDist >= ZSTD_WINDOW_START_INDEX. */
U32 const currentCycleCorrection = currentCycle < ZSTD_WINDOW_START_INDEX
? MAX(cycleSize, ZSTD_WINDOW_START_INDEX)
: 0;
U32 const newCurrent = currentCycle
+ currentCycleCorrection
+ MAX(maxDist, cycleSize);
+ MAX(roundedMaxDist, cycleSize);
U32 const correction = curr - newCurrent;
/* maxDist must be a power of two so that:
* (newCurrent & cycleMask) == (curr & cycleMask)
* This is required to not corrupt the chains / binary tree.
*
* Now that window sizes can be non-power-of-two, we round it up to the
* next power of two.
*/
assert((maxDist & (maxDist - 1)) == 0);
assert(roundedMaxDist >= maxDist);
assert(roundedMaxDist < maxDist + 7 * (maxDist >> 3));
assert((roundedMaxDist & (roundedMaxDist - 1)) == 0);
assert((curr & cycleMask) == (newCurrent & cycleMask));
assert(curr > newCurrent);
if (!ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY) {
Expand Down Expand Up @@ -1392,9 +1486,9 @@ U32 ZSTD_window_update(ZSTD_window_t* window,
/**
* Returns the lowest allowed match index. It may either be in the ext-dict or the prefix.
*/
MEM_STATIC U32 ZSTD_getLowestMatchIndex(const ZSTD_MatchState_t* ms, U32 curr, unsigned windowLog)
MEM_STATIC U32 ZSTD_getLowestMatchIndex(const ZSTD_MatchState_t* ms, U32 curr)
{
U32 const maxDistance = 1U << windowLog;
U32 const maxDistance = ZSTD_windowSize(ms->cctxParams);
U32 const lowestValid = ms->window.lowLimit;
U32 const withinWindow = (curr - lowestValid > maxDistance) ? curr - maxDistance : lowestValid;
U32 const isDictionary = (ms->loadedDictEnd != 0);
Expand All @@ -1409,9 +1503,9 @@ MEM_STATIC U32 ZSTD_getLowestMatchIndex(const ZSTD_MatchState_t* ms, U32 curr, u
/**
* Returns the lowest allowed match index in the prefix.
*/
MEM_STATIC U32 ZSTD_getLowestPrefixIndex(const ZSTD_MatchState_t* ms, U32 curr, unsigned windowLog)
MEM_STATIC U32 ZSTD_getLowestPrefixIndex(const ZSTD_MatchState_t* ms, U32 curr)
{
U32 const maxDistance = 1U << windowLog;
U32 const maxDistance = ZSTD_windowSize(ms->cctxParams);
U32 const lowestValid = ms->window.dictLimit;
U32 const withinWindow = (curr - lowestValid > maxDistance) ? curr - maxDistance : lowestValid;
U32 const isDictionary = (ms->loadedDictEnd != 0);
Expand Down Expand Up @@ -1538,13 +1632,13 @@ BlockSummary ZSTD_get1BlockSummary(const ZSTD_Sequence* seqs, size_t nbSeqs);
* These prototypes shall only be called from within lib/compress
* ============================================================== */

/* ZSTD_getCParamsFromCCtxParams() :
/* ZSTD_fillCParamsInCCtxParams() :
* cParams are built depending on compressionLevel, src size hints,
* LDM and manually set compression parameters.
* Note: srcSizeHint == 0 means 0!
*/
ZSTD_compressionParameters ZSTD_getCParamsFromCCtxParams(
const ZSTD_CCtx_params* CCtxParams, U64 srcSizeHint, size_t dictSize, ZSTD_CParamMode_e mode);
void ZSTD_fillCParamsInCCtxParams(
ZSTD_CCtx_params* cctxParams, U64 srcSizeHint, size_t dictSize, ZSTD_CParamMode_e mode);

/*! ZSTD_initCStream_internal() :
* Private use only. Init streaming operation.
Expand All @@ -1562,6 +1656,11 @@ void ZSTD_resetSeqStore(SeqStore_t* ssPtr);
* as the name implies */
ZSTD_compressionParameters ZSTD_getCParamsFromCDict(const ZSTD_CDict* cdict);

/*! ZSTD_checkCCtxCParams_internal() :
* Checks the CParams in the CCtxParams (including related parameters not
* *actually* stored in the CParams struct). */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe name them ?
It seems to be limited to params->windowFrac at this point.

size_t ZSTD_checkCCtxCParams_internal(const ZSTD_CCtx_params* params);

/* ZSTD_compressBegin_advanced_internal() :
* Private use only. To be called from zstdmt_compress.c. */
size_t ZSTD_compressBegin_advanced_internal(ZSTD_CCtx* cctx,
Expand Down
Loading
Loading