[huf] Reduce stack usage of HUF_readDTableX2 by ~972 bytes #2524
Conversation
* Use `HUF_readStats_wksp()` * Use workspace in `HUF_fillDTableX2*()` * Clean up workspace usage to use a workspace struct
terrelln
force-pushed
the
huf-stack-reduction
branch
from
9f16c71
to
3b1aba4
Compare
* Move `counting` into the workspace * Inrease `HUF_DECOMPRESS_WORKSPACE_SIZE` by 512 bytes
terrelln
changed the title
terrelln
changed the title
|
This is a good saving, and a nice refactor, especially when employing |
* Move `counting` to a struct in `FSE_decompress_wksp_body()` * Fix error code in `FSE_decompress_wksp_body()` * Rename a variable in `HUF_ReadDTableX2_Workspace`
|
@Cyan4973 is this good to go? |
lib/common/fse_decompress.c
Outdated
| @@ -310,6 +310,11 @@ size_t FSE_decompress_wksp(void* dst, size_t dstCapacity, const void* cSrc, size | |||
| return FSE_decompress_wksp_bmi2(dst, dstCapacity, cSrc, cSrcSize, maxLog, workSpace, wkspSize, /* bmi2 */ 0); | |||
| } | |||
|
|
|||
| typedef struct { | |||
| short count[FSE_MAX_SYMBOL_VALUE + 1]; | |||
| } FSE_NormalizedCount; | |||
There was a problem hiding this comment.
very minor :
I think I wasn't specific enough in my previous comment:
I was expecting a second struct member FSE_DTable dtable[1].
The point was to avoid pointer arithmetic to determine dtable.
Thus, instead of FSE_DTable* const dtable = (FSE_DTable*)(void*)((BYTE*)workSpace + sizeof(*ncount));,
it would rather be FSE_DTable* const dtable = &ncount.dtable;.
Having a last member of a struct as an array of size 1 is a known trick in C.
As the size is not enforced once the array decays as a pointer, it can effectively represent an array of any size.
Of course, sizeof(struct) is no longer correct then, since it's unaware of the real size of the array.
It's also not possible to stack anything after the array.
Some people prefer using an array of size [0] for this purpose,
which is indeed nice to make sizeof(struct) represent the size of members before the variable size array.
However, I read here and there that it could cause compatibility concerns with "some" compilers (though I don't know which one), hence I tend to use [1] to avoid such issue.
That being said, another consensus was that the size should represent the minimum expected size of the array,
thus [0] is relevant for an optional array that may be empty.
There was a problem hiding this comment.
Sure, I wasn't sure if 0-sized arrays were a standard compliant feature or not. But I can give it a go and see if anything in CI complains.
|
Yes, that's fine. |
terrelln
force-pushed
the
huf-stack-reduction
branch
from
a7801de
to
634bfd3
Compare
HUF_readStats_wksp()HUF_fillDTableX2*()FSE_decompress_wksp()into the workspace.HUF_DECOMPRESS_WORKSPACE_SIZEby 512 bytes to accommodate the new array.This reduces the decompression high water mark by 972 bytes.