Skip to content

Backport upstream LDM size-estimation fix #359

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.

Sign up for GitHub

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

Open
yousefissa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Backport upstream LDM size-estimation fix #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions docs/news.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ Version History
next (not yet released)
=======================

* Backported `zstd's LDM size-estimation fix
<https://github.com/facebook/zstd/commit/81cf153bce7c0ab41aeef574a70e32eaea7a8ba7>`_
so ``estimated_compression_context_size()`` resolves default LDM parameters
instead of dividing by zero when ``enable_ldm=True``. (#345)

Backwards Compatibility Notes
-----------------------------

Expand Down
44 changes: 44 additions & 0 deletions tests/test_data_structures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,50 @@ def test_estimated_compression_context_size(self):
p.estimated_compression_context_size(), 1303304, delta=4000
)

def test_estimated_compression_context_size_ldm(self):
# Resolved LDM defaults for a 1 MiB window, including a hash-log
# override and a strategy with a different default match length.
cases = [
(zstd.STRATEGY_DFAST, {}, (13, 64, 4, 7)),
(zstd.STRATEGY_DFAST, {"ldm_hash_log": 16}, (16, 64, 4, 4)),
(zstd.STRATEGY_BTULTRA2, {}, (16, 32, 8, 4)),
]
names = (
"ldm_hash_log",
"ldm_min_match",
"ldm_bucket_size_log",
"ldm_hash_rate_log",
)

for strategy, overrides, defaults in cases:
with self.subTest(strategy=strategy, overrides=overrides):
kwargs = {"window_log": 20, "strategy": strategy}
params = zstd.ZstdCompressionParameters.from_level(
3, enable_ldm=True, **kwargs, **overrides
)
explicit = zstd.ZstdCompressionParameters.from_level(
3, enable_ldm=True, **kwargs, **dict(zip(names, defaults))
)
disabled = zstd.ZstdCompressionParameters.from_level(
3, enable_ldm=False, **kwargs
)
before = tuple(getattr(params, name) for name in names)

size = params.estimated_compression_context_size()
self.assertEqual(
size, explicit.estimated_compression_context_size()
)
self.assertGreater(
size, disabled.estimated_compression_context_size()
)
self.assertEqual(
tuple(getattr(params, name) for name in names), before
)
self.assertEqual(params.enable_ldm, 1)
self.assertEqual(
params.estimated_compression_context_size(), size
)

def test_strategy(self):
p = zstd.ZstdCompressionParameters(strategy=2)
self.assertEqual(p.strategy, 2)
Expand Down
12 changes: 10 additions & 2 deletions zstd/zstd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24709,15 +24709,19 @@ size_t ZSTD_estimateCCtxSize_usingCCtxParams(const ZSTD_CCtx_params* params)
{
ZSTD_compressionParameters const cParams =
ZSTD_getCParamsFromCCtxParams(params, ZSTD_CONTENTSIZE_UNKNOWN, 0, ZSTD_cpm_noAttachDict);
ldmParams_t ldmParams = params->ldmParams;
ZSTD_ParamSwitch_e const useRowMatchFinder = ZSTD_resolveRowMatchFinderMode(params->useRowMatchFinder,
&cParams);

RETURN_ERROR_IF(params->nbWorkers > 0, GENERIC, "Estimate CCtx size is supported for single-threaded compression only.");
if (ldmParams.enableLdm == ZSTD_ps_enable) {
ZSTD_ldm_adjustParameters(&ldmParams, &cParams);
}
/* estimateCCtxSize is for one-shot compression. So no buffers should
* be needed. However, we still allocate two 0-sized buffers, which can
* take space under ASAN. */
return ZSTD_estimateCCtxSize_usingCCtxParams_internal(
&cParams, &params->ldmParams, 1, useRowMatchFinder, 0, 0, ZSTD_CONTENTSIZE_UNKNOWN, ZSTD_hasExtSeqProd(params), params->maxBlockSize);
&cParams, &ldmParams, 1, useRowMatchFinder, 0, 0, ZSTD_CONTENTSIZE_UNKNOWN, ZSTD_hasExtSeqProd(params), params->maxBlockSize);
}

size_t ZSTD_estimateCCtxSize_usingCParams(ZSTD_compressionParameters cParams)
Expand Down Expand Up @@ -24767,6 +24771,7 @@ size_t ZSTD_estimateCStreamSize_usingCCtxParams(const ZSTD_CCtx_params* params)
RETURN_ERROR_IF(params->nbWorkers > 0, GENERIC, "Estimate CCtx size is supported for single-threaded compression only.");
{ ZSTD_compressionParameters const cParams =
ZSTD_getCParamsFromCCtxParams(params, ZSTD_CONTENTSIZE_UNKNOWN, 0, ZSTD_cpm_noAttachDict);
ldmParams_t ldmParams = params->ldmParams;
size_t const blockSize = MIN(ZSTD_resolveMaxBlockSize(params->maxBlockSize), (size_t)1 << cParams.windowLog);
size_t const inBuffSize = (params->inBufferMode == ZSTD_bm_buffered)
? ((size_t)1 << cParams.windowLog) + blockSize
Expand All @@ -24776,8 +24781,11 @@ size_t ZSTD_estimateCStreamSize_usingCCtxParams(const ZSTD_CCtx_params* params)
: 0;
ZSTD_ParamSwitch_e const useRowMatchFinder = ZSTD_resolveRowMatchFinderMode(params->useRowMatchFinder, &params->cParams);

if (ldmParams.enableLdm == ZSTD_ps_enable) {
ZSTD_ldm_adjustParameters(&ldmParams, &cParams);
}
return ZSTD_estimateCCtxSize_usingCCtxParams_internal(
&cParams, &params->ldmParams, 1, useRowMatchFinder, inBuffSize, outBuffSize,
&cParams, &ldmParams, 1, useRowMatchFinder, inBuffSize, outBuffSize,
ZSTD_CONTENTSIZE_UNKNOWN, ZSTD_hasExtSeqProd(params), params->maxBlockSize);
}
}
Expand Down