Skip to content

Add Functions that Report LZ4F Contexts' Memory Footprints #1595

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 2, 2025
Merged

Add Functions that Report LZ4F Contexts' Memory Footprints #1595

merged 2 commits into from
Jun 2, 2025

Conversation

felixhandte
Copy link
Member

No description provided.

@Cyan4973
Copy link
Member

The CI test issue has been fixed in #1602.
If you rebase your PR on top of dev, it should fix the test errors.

@Cyan4973 Cyan4973 self-assigned this Jun 1, 2025
@Cyan4973
Copy link
Member

Cyan4973 commented Jun 1, 2025

Any plan to complete this PR ?
It seems it mostly deserves a rebase to get back to clean CI status,
and then fix the rest from there, if there is any remaining issue.

@felixhandte felixhandte force-pushed the ctx-size-funcs branch 3 times, most recently from 7a07e39 to a844645 Compare June 2, 2025 14:49
@felixhandte
Add Test Validating Allocated Size Matches Reported Size
063d1a9 
Adds additional record-keeping to track the live allocation size of a context
using a custom memory allocator.

This could be extended to more scenarios (e.g., streaming) if desired.
Cyan4973
Cyan4973 approved these changes Jun 2, 2025
View reviewed changes
Copy link
Member

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface and implementation are both clear and well implemented.

All the maintenance complexity is concentrated in the test case.
The risk here is that, if something breaks this test in the future, then understanding what the test does and why it fails, and therefore what needs to be fixed, can be a bit challenging.

I don't have a different test to offer, I just wonder if understanding the test can be made easier for an external reviewer, notably if it happens to break in the future (typically as a consequence of some (possibly erroneous) future change).

@felixhandte
Copy link
Member Author

@Cyan4973, I actually expect the test to be fairly robust: it tracks the actual live memory that an object manages, via the Test_alloc_state / lz4f_cmem_test, and compares that against the memory usage that these new APIs report. In the event that these diverge, it should be high signal that either:

  1. You don't intend to change the allocations owned by the context objects, and your changes are leaking memory, which this is detecting.
  2. You did intend to change the allocations owned by the context objects, and you need to update the size-reporting implementations to reflect those changes.

In both cases I think it's pretty clear. You'll get a message: "%llu allocated in cctx but it says its size is %llu.\n".

@Cyan4973 Cyan4973 merged commit 2bc386d into lz4:dev Jun 2, 2025
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees

@Cyan4973 Cyan4973

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@felixhandte @Cyan4973