-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add init once memory (#3528) #3529
Conversation
Note: I explore reducing the scope of changes in this path by maintaining the buffer space. |
lib/compress/zstd_cwksp.h
Outdated
@@ -310,7 +331,7 @@ ZSTD_cwksp_internal_advance_phase(ZSTD_cwksp* ws, ZSTD_cwksp_alloc_phase_e phase | |||
ws->tableEnd = objectEnd; /* table area starts being empty */ | |||
if (ws->tableValidEnd < ws->tableEnd) { | |||
ws->tableValidEnd = ws->tableEnd; | |||
} } } | |||
} } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: doesn't seem to be the proper bracket alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/compress/zstd_cwksp.h
Outdated
* 2. Another initOnce buffer that has been allocated before (and so was previously memset) | ||
* 3. An ASAN redzone, in which case we don't want to write on it | ||
* For these reasons it should be fine to not explicitly zero every byte up to ws->initOnceStart. | ||
* Note that we assume here tha MSAN and ASAN cannot run in the same time. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we assume here that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/compress/zstd_compress.c
Outdated
@@ -1932,6 +1932,17 @@ ZSTD_reset_matchState(ZSTD_matchState_t* ms, | |||
ZSTD_cwksp_clean_tables(ws); | |||
} | |||
|
|||
if (ZSTD_rowMatchFinderUsed(cParams->strategy, useRowMatchFinder)) { | |||
/* Row match finder needs an additional table of hashes ("tags") */ | |||
size_t const tagTableSize = hSize * sizeof(U16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigate if we really need U16
space for the tags table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we need it
@Cyan4973 - see CR fixes |
baefad5
to
70d69dc
Compare
- Adds memory type that is guaranteed to have been initialized at least once in the workspace's lifetime. - Changes tag space in row hash to be based on init once memory. - Moves buffers to aligned memory and removes the buffer memory type.
…o allocate them after aligned buffers
70d69dc
to
f4aab97
Compare
Part 1 of #3528
Update: now maintains the usage of unaligned buffers