Skip to content

Compress in-memory slices with Zstd #2268

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

Merged
merged 10 commits into from
May 19, 2022

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented May 6, 2022

In a repeated test with 8'557'668 ingested Suricata events enabling the compression had a negligible effect on import speed (under 0.5%), but reduced the size of the archive and also the in-memory size of the loaded slices by roughly the same amount.

Here are the sizes of the database for three scenarios: Uncompressed, Zstd with the default compression level, and Zstd with the max compression level (50% slower on import, so irrelevant in the grand scheme of things).

❯ du -csh vast.db.uncompressed/{index,archive}
667M	vast.db.uncompressed/index
1.8G	vast.db.uncompressed/archive
2.5G	total

❯ du -csh vast.db.zstd.default/{index,archive}
668M	vast.db.zstd.default/index
426M	vast.db.zstd.default/archive
1.1G	total

❯ du -csh vast.db.zstd.max/{index,archive}
675M	vast.db.zstd.max/index
361M	vast.db.zstd.max/archive
1.0G	total

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Discuss whether we want to enable this by default, or make this an option, or just not do it at all. After that comes testing and a changelog entry.

@dominiklohmann dominiklohmann added the performance Improvements or regressions of performance label May 6, 2022
@mavam
Copy link
Member

mavam commented May 6, 2022

Out of curiosity, trying to understand the entropy: can you zstd-compress the index (on the filesystem) as well and report on the sizes?

@dominiklohmann
Copy link
Member Author

Out of curiosity, trying to understand the entropy: can you zstd-compress the index (on the filesystem) as well and report on the sizes?

667M to 229M when doing it per-file.

@mavam
Copy link
Member

mavam commented May 6, 2022

667M to 229M when doing it per-file.

So down to ~34% of the original size.

@dispanser
Copy link
Contributor

dispanser commented May 6, 2022

I guess it would be an easy win if this could be made configurable. There seem to be people out there using a compressed file system with VAST, and for those scenarios the additional compression might become a performance degradation, as there would be two layers attempting to compress the data.

But still, an impressive win for such a small patch 👍

@tobim
Copy link
Member

tobim commented May 6, 2022

Very nice! I would like to measure the export performance and RSS as well.

There seem to be people out there using a compressed file system with VAST, and for those scenarios the additional compression might become a performance degradation, as there would be two layers attempting to compress the data.

Probably not even necessary, at least btrfs opts out of compression very quickly if it doesn't manage to compress the first few kB itself. This should also be much more effective than compressing individual filesystem blocks.

Out of curiosity, trying to understand the entropy: can you zstd-compress the index (on the filesystem) as well and report on the sizes?

667M to 229M when doing it per-file.

That is promising, but we should probably do it a per-value-index basis instead of the whole file.

@dominiklohmann
Copy link
Member Author

I guess it would be an easy win if this could be made configurable.

Yes, I think so. Even with Parquet stores coming up this is still valuable to reduce the memory usage.

Very nice! I would like to measure the export performance and RSS as well.

Will do and report back!

Out of curiosity, trying to understand the entropy: can you zstd-compress the index (on the filesystem) as well and report on the sizes?

667M to 229M when doing it per-file.

That is promising, but we should probably do it a per-value-index basis instead of the whole file.

I think that's for a separate story, let's keep this focussed on the table slices only for now.

@tobim
Copy link
Member

tobim commented May 6, 2022

I think that's for a separate story, let's keep this focussed on the table slices only for now.

Definitely.

@dominiklohmann
Copy link
Member Author

Very nice! I would like to measure the export performance

No noticeable difference for Zstd with default compression. Unpacking is probably slightly more expensive, but sending things over the wire is much cheaper. The higher compression level is especially bad when selecting subsets of table slices, because that requires re-compressing the results before sending them over with the current approach. But from this point of view alone I don't see why we'd not just always enable Zstd with default compression.

and RSS as well.

For the Zstd default during export:

Maximum resident set size (kbytes): 2186784
Socket messages sent: 2818
Socket messages received: 584

For the uncompressed version:

Maximum resident set size (kbytes): 2406112
Socket messages sent: 19274
Socket messages received: 574

In a repeated test with 8'557'668 ingested Suricata events enabling the
compression had a negligible effect on import speed (under 0.5%), but reduced
the size of the archive and also the in-memory size of the loaded slices by
roughly the same amount.

Here are the sizes of the database for three scenarios: Uncompressed, Zstd with
the default compression level, and Zstd with the max compression level (50%
slower on important, so irrelevant in the grand scheme of things).

```
❯ du -csh vast.db.uncompressed/{index,archive}
667M	vast.db.uncompressed/index
1.8G	vast.db.uncompressed/archive
2.5G	total

❯ du -csh vast.db.zstd.default/{index,archive}
668M	vast.db.zstd.default/index
426M	vast.db.zstd.default/archive
1.1G	total

❯ du -csh vast.db.zstd.max/{index,archive}
675M	vast.db.zstd.max/index
361M	vast.db.zstd.max/archive
1.0G	total
```
@dominiklohmann dominiklohmann force-pushed the topic/zstd-compress-by-default branch from 10da7ab to 2396062 Compare May 16, 2022 16:43
@dominiklohmann dominiklohmann requested review from mavam and a team May 16, 2022 16:47
Now that we have Zstd-compression for table slices we can (1) increase the
partition size 4-fold without running into size problems with the FlatBuffers
builders, and (2) can adjust the default table slice size upwards which is a
huge boost to overall efficiency.
@dominiklohmann
Copy link
Member Author

I've extended this PR with the tuned defaults following my extensive measurements on our testbed server.

Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

I think we can make these values accessible to those that haven't burnt powers of two in their heads.

Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

This has been extensively tested and co-reviewed by @dispanser.

@dominiklohmann dominiklohmann enabled auto-merge May 19, 2022 18:48
@dominiklohmann dominiklohmann merged commit 3e8fcb7 into master May 19, 2022
@dominiklohmann dominiklohmann deleted the topic/zstd-compress-by-default branch May 19, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improvements or regressions of performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants