Memory managment in init_stream (init_bitstream on feat/api branch) and terminate stream.

Gregor Weiss commented

2024-07-01 12:18:21 +00:00

Collaborator

The functions init_bitstream and terminate_stream respectively allocate and reallocate the bitstream->memory depending on whether a NULL is passed into init_bitstream and if the L and Lmax differ in terminate_stream.

Now, given the newer API on the feat/api branch, in which the user provides a pointer to buffer the (de-)compression output, the behavior of terminate_stream might lead to reallocating the user-provided pointer.

A simpler and safer approach should be chosen for both function possibly following RAII or other memory managment schemes as suggested here: https://www.rfleury.com/p/untangling-lifetimes-the-arena-allocator

PR #41

The functions `init_bitstream` and `terminate_stream` respectively allocate and reallocate the bitstream->memory depending on whether a NULL is passed into `init_bitstream` and if the `L` and `Lmax` differ in `terminate_stream`. Now, given the newer API on the feat/api branch, in which the user provides a pointer to buffer the (de-)compression output, the behavior of `terminate_stream` might lead to reallocating the user-provided pointer. A simpler and safer approach should be chosen for both function possibly following RAII or other memory managment schemes as suggested here: https://www.rfleury.com/p/untangling-lifetimes-the-arena-allocator PR #41

Gregor Weiss added this to the Maintainbarkeit project 2024-07-01 12:18:21 +00:00

Patrick Vogler commented

2024-07-01 17:21:09 +00:00

Owner

Reallocation of the memory block should only happen during code-stream and header assembly (AFAIK). Are we going to use the memory block provided by the user to initialize the respective bit-streams?

Gregor Weiss commented

2024-07-01 18:57:16 +00:00

Author

Collaborator

AFAYK. Yes, you are right. The user output memory is used to initialize a bitstream during decompression. (See 363846202b/src/library/codestream.c (L1398)) So, only the header assembly in tier2 requires allocation (363846202b/src/library/tier2.c (L485)). An option is to pull it out and pass the locally allocated memory into the init_stream. It would be preferable in the sense of SRP.

For terminate_stream, an equivalent solution that separates freeing the memory from passing it on to the packed stream would be possible. SRP would clarify memory management.

Additionally, looking into this revealed that type bitstream instances are local in function scope. Dynamic allocation and pointer-value semantics could be omitted. Then, there would be no allocation to deal with bitstreams whatsoever.

AFAYK. Yes, you are right. The user output memory is used to initialize a `bitstream` during decompression. (See https://code.hlrs.de/TOPIO/BigWhoop/src/commit/363846202bcb1b18505581f8d21ff5bdbe465831/src/library/codestream.c#L1398) So, only the header assembly in tier2 requires allocation (https://code.hlrs.de/TOPIO/BigWhoop/src/commit/363846202bcb1b18505581f8d21ff5bdbe465831/src/library/tier2.c#L485). An option is to pull it out and pass the locally allocated memory into the `init_stream`. It would be preferable in the sense of SRP. For `terminate_stream`, an equivalent solution that separates freeing the memory from passing it on to the packed stream would be possible. SRP would clarify memory management. Additionally, looking into this revealed that type `bitstream` instances are local in function scope. Dynamic allocation and pointer-value semantics could be omitted. Then, there would be no allocation to deal with bitstreams whatsoever.

Gregor Weiss self-assigned this 2024-07-01 18:59:39 +00:00

Gregor Weiss referenced this issue

2024-07-02 16:45:16 +00:00

Cleaned memory management for bitstream #41

Memory managment in init_stream (init_bitstream on feat/api branch) and terminate stream. #36