Follow-up zstd fixes #22
Conversation
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
zstd-decompress.c
Outdated
| @@ -66,37 +49,59 @@ int zstd_decompress_file(const char *filename) | |||
| } | |||
| close(input_file_fd); | |||
|
|
|||
| ZSTD_DCtx *zstd_context = ZSTD_createDCtx(); | |||
There was a problem hiding this comment.
If you don't want to keep the context around, you can just use the non-context version of ZSTD_decompressDCtx(). But what are you trying to achieve with this? The commit message doesn't have any rationale.
There was a problem hiding this comment.
Good point - will swap for ZSTD_decompress().
The goal is to consolidate zstd handling in a single place and avoid calling into zstd unless needed. In addition it makes fixing Android bit friendlier.
Does the designated ZSTD_createDCtx() bring notable benefit? In theory it's identical to malloc(128k) which is quite reasonable IMHO.
There was a problem hiding this comment.
I used the context variant because the documentation made it sound like it made sense for this use-case as we would be doing multiple decompressions, so I wrote a comment. @lumag then indicated he would like to see this use it: #20 (comment). However, I didn't do any testing to investigate how this actually would affect performance and like I said I just blindly went off what the docs said. If others prefer not using the context approach, I'm fine with changing it.
There was a problem hiding this comment.
So it's the typical trade-off of ahead-of-time vs on-demand.
Personally I'm not sure if (how much) the former makes sense for single-threaded daemon, esp. since it's a notable amount and might not be needed in the first place. Not my call either way.
Let's wait for @lumag's input
There was a problem hiding this comment.
Granted that tqftpserv handles only few files during the whole lifetime and most of devices don't have swap, I think it's fine to use non-context versions of the functions. Let's free the memory for other software.
There was a problem hiding this comment.
Thanks, updated. Sorry for missing your earlier comment 🙇
| @@ -15,7 +15,6 @@ | |||
|
|
|||
| #include "list.h" | |||
| #include "translate.h" | |||
| #include "zstd-decompress.h" | |||
There was a problem hiding this comment.
Please add a sensible commit message. For other commits it a 'might be better', for this one a proper commit messages explaining the reasons for the change is a must. Would it take more time to create a context each time? Can the context be shared between invocations or not?
zstd-decompress.c
Outdated
| @@ -66,37 +49,59 @@ int zstd_decompress_file(const char *filename) | |||
| } | |||
| close(input_file_fd); | |||
|
|
|||
| ZSTD_DCtx *zstd_context = ZSTD_createDCtx(); | |||
There was a problem hiding this comment.
Granted that tqftpserv handles only few files during the whole lifetime and most of devices don't have swap, I think it's fine to use non-context versions of the functions. Let's free the memory for other software.
Currently we pre-allocate around 128K of zstd state ahead of time,
keeping if for the whole daemon lifetime... Even if we don't need to
decompress any files. As mentioned by Dmitry:
Granted that tqftpserv handles only few files during the whole
lifetime and most of devices don't have swap, I think it's fine to
use non-context versions of the functions. Let's free the memory for
other software.
Swap ZSTD_decompressDCtx() for ZSTD_decompress() which will allocate and
free the state when needed.
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Bit of a blind fix, since I don't have an Android dev setup nearby. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
evelikov
force-pushed
the
zstd-followups
branch
from
bec1b82
to
f9da066
Compare
Addresses some issues with the initial introduction #20
/ccing @lumag @Newbytee @TravMurav