[lib] Move some ZSTD_CCtx_params off the stack #2615
Conversation
terrelln
force-pushed
the
stack-space
branch
2 times, most recently
from
798630a
to
eb7e74c
Compare
* Take `params` by const reference in `ZSTD_resetCCtx_internal()`. * Add `simpleApiParams` to the CCtx and use them in the simple API functions, instead of creating those parameters on the stack. I think this is a good direction to move in, because we shouldn't need to worry about adding parameters to `ZSTD_CCtx_params`, since it should always be on the heap (unless they become absoultely gigantic). Some `ZSTD_CCtx_params` are still on the stack in the CDict functions, but I've left them for now, because it was a little more complex, and we don't use those functions in stack-constrained currently.
This allows us to quickly check for compile errors in debug log messages, which are compiled out when `DEBUGLEVEL < 2`.
| @@ -24,7 +24,7 @@ PRGDIR = ../programs | |||
| PYTHON ?= python3 | |||
| TESTARTEFACT := versionsTest | |||
|
|
|||
| DEBUGLEVEL ?= 1 | |||
| DEBUGLEVEL ?= 2 | |||
There was a problem hiding this comment.
Set
DEBUGLEVEL=2by default intests/Makefileto catch compiler errors inDEBUGLOG()statements.
There (is ? / used to be ?) a CI test with DEBUGLEVEL set to 2 specifically for this purpose (check DEBUGLOG() statements).
The question is :
should checking validity of DEBUGLOG() statement be the purpose of a specific CI test,
rather than default for any compilation within tests/ ?
My concern is that traces from DEBUGLEVEL = 2 could impact the generated binary layout more than just assert().
There was a problem hiding this comment.
There (is ? / used to be ?) a CI test with DEBUGLEVEL set to 2 specifically for this purpose (check DEBUGLOG() statements).
I added this because only Appveyor caught it.
My concern is that traces from DEBUGLEVEL = 2 could impact the generated binary layout more than just assert().
Does it matter for test cases?
There was a problem hiding this comment.
Does it matter for test cases?
I'm not sure.
I was concerned that it could make a sensible performance difference,
reducing the nb of fuzzer tests performed within a given timeframe.
It could also be that the difference is not that large.
There was a problem hiding this comment.
So I went ahead and tested the performance difference between DEBUGLEVEL=1 and DEBUGLEVEL=2 on a given test workload : time ./fuzzer -v -i1000 -t1 -s7653
There was a performance impact, but I believe it's small enough, < 5% (time went up from 22.3s to 23.2s).
paramsby const reference inZSTD_resetCCtx_internal().simpleApiParamsto the CCtx and use them in the simple APIfunctions, instead of creating those parameters on the stack.
DEBUGLEVEL=2by default intests/Makefileto catch compilererrors in
DEBUGLOG()statements.I think this is a good direction to move in, because we shouldn't need
to worry about adding parameters to
ZSTD_CCtx_params, since it shouldalways be on the heap (unless they become absoultely gigantic).
Some
ZSTD_CCtx_paramsare still on the stack in the CDict functions,but I've left them for now, because it was a little more complex, and we
don't use those functions in stack-constrained currently.