Introduce delta snapshot utilities and zstd #81
Conversation
Squashed commits: Read/write basic types as bytes into util Delta encoding utilities including zstd Fix linking errors with zstd Add delta encoding to log Don't encode unchanged all-zero pages Fix from-scratch build with zstd Cleanup for PR FetchContent_MakeAvailable actually requires cmake 3.14 Clang-format the updated files
There was a problem hiding this comment.
Hey @KubaSz , thanks for this! I am leaving the bulk of the review for Simon, but just pointed out one minor thing as I skimmed through the code.
| @@ -28,7 +28,7 @@ include(cmake/ExternalProjects.cmake) | |||
| function(faabric_lib lib_name lib_deps) | |||
| # "Normal" library used for linking internally | |||
| add_library(${lib_name} ${lib_deps}) | |||
| target_include_directories(${lib_name} | |||
There was a problem hiding this comment.
clang-format removes trailing whitespaces, but we have no linting for CMake files. Thus the unpleasant and misleading diff. I agree though that removing trailing whitespaces is nice. Should we "unofficially" enforce this ourselves @Shillaker ?
There was a problem hiding this comment.
Yes I think it's great that Jakub is cleaning up all this unnecessary whitespace 😂 . We'll have to do a big clear-up ourselves but this can definitely stay as a start.
| @@ -26,6 +26,7 @@ class SystemConfig | |||
| std::string captureStdout; | |||
| std::string stateMode; | |||
| std::string wasmVm; | |||
| std::string deltaSnapshotEncoding; | |||
There was a problem hiding this comment.
Could you add a very simple test for setting and unsetting this value in tests/test/util/test_config.cpp?
There was a problem hiding this comment.
Yes that's the only thing I'd add, but I'll be doing some work on this so can add it myself.
| @@ -28,7 +28,7 @@ include(cmake/ExternalProjects.cmake) | |||
| function(faabric_lib lib_name lib_deps) | |||
| # "Normal" library used for linking internally | |||
| add_library(${lib_name} ${lib_deps}) | |||
| target_include_directories(${lib_name} | |||
There was a problem hiding this comment.
Yes I think it's great that Jakub is cleaning up all this unnecessary whitespace 😂 . We'll have to do a big clear-up ourselves but this can definitely stay as a start.
| @@ -26,6 +26,7 @@ class SystemConfig | |||
| std::string captureStdout; | |||
| std::string stateMode; | |||
| std::string wasmVm; | |||
| std::string deltaSnapshotEncoding; | |||
There was a problem hiding this comment.
Yes that's the only thing I'd add, but I'll be doing some work on this so can add it myself.
|
LGTM 👍 |
This introduces a zstd dependency and adds configurable delta snapshots with multiple encodings.
The different encoding formats (with/without pages, compressed/uncompressed, xor/overwrite mode) are tested to work, but I have not determined their individual overheads and benefits so far.