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

Avoid Clearing Tables Even When Changing CParams #1780

Merged
merged 21 commits into from
Sep 16, 2019

Conversation

felixhandte
Copy link
Contributor

@felixhandte felixhandte commented Sep 10, 2019

This changeset builds on the work in #1712, and uses the new workspace abstraction introduced in that PR to perform more efficient context resets, even when the compression parameters change.

Overall this PR should produce compression speed wins for repeated compressions on contexts where the parameters vary (whether because the compression level changes, or the size of the input changes), and be otherwise perf neutral. The gains are very situational, and range from 0% to 4%. There are also outsize wins that this PR brings from fixing existing pathological cases, like constantly switching compression levels on streaming compression of small responses without a pledged src size, which range up 40x improvements.

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.
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 Show resolved Hide resolved
lib/compress/zstd_compress.c Show resolved Hide resolved
lib/compress/zstd_cwksp.h Show resolved Hide resolved
Copy link
Contributor

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

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

LGTM,
is there an ASAN follow up planned for this PR ?

@felixhandte
Copy link
Contributor Author

@Cyan4973, I believe @terrelln volunteered to look into that as a follow-up. So I don't expect to add anything further to this PR itself.

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

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

LGTM after we fix the missing MSAN header!

lib/common/mem.h Outdated Show resolved Hide resolved
…rovide It

Instead, explicitly declare the functions we use.
@felixhandte felixhandte merged commit 2164a13 into facebook:dev Sep 16, 2019
QrczakMK added a commit to google/riegeli that referenced this pull request Nov 8, 2019
Apply other changes enabled by this update:

Use `ZSTD_CCtx_setParameter(ZSTD_c_srcSizeHint)` for specifying
`size_hint`, instead of `ZSTD_createCCtxParams()` +
`ZSTD_getParams()` + `ZSTD_CCtxParams_init_advanced()` +
`ZSTD_CCtx_setParametersUsingCCtxParams()`.
This was introduced by facebook/zstd#1733

Do not use `ZSTD_CCtxKey`, recycle compressors without comparing
compression options. This is fast after
facebook/zstd#1780

Do not create the zstd compressor lazily. It is no longer needed because
`size_hint` no longer participates in `ZSTD_CCtxKey`.

PiperOrigin-RevId: 279266067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants