Skip to content
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

chore: enable heap allocations for lz4 context #4295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kostasrim
Copy link
Contributor

@kostasrim kostasrim commented Dec 11, 2024

The issue is that lz4 allocates the context on the stack. For that it uses 16kb only for its internal buffers (not accounting the operations and the actual stack size of the variables of each function in the stack). In dragonfly the fiber stack is 32kb which can cause an overflow.

This is a temporary solution since Roman suggested to manually create the context and pass it down directly to the corresponding lz4 functions. Since LZ4F_compressFrame uses internally some functions that are not exposed as a public API.

I will use this branch also for testing. From there we can either merge this and then improve on adding the manual context handling or do directly just that. Either way I open this PR.

Resolves #4245

@kostasrim kostasrim self-assigned this Dec 11, 2024
@romange
Copy link
Collaborator

romange commented Dec 11, 2024

@kostasrim what are the functions that are not exposed?

@romange
Copy link
Collaborator

romange commented Dec 12, 2024

btw, I've actually used lz4 in other universe, here is an example of how to do it properly with lz4 context:

https://github.com/romange/gaia/blob/master/file/lz4_file.cc#L34

I suggest to keep this PR pending for now, and I may be able to fix it some time next week.

@kostasrim
Copy link
Contributor Author

kostasrim commented Dec 12, 2024

@kostasrim what are the functions that are not exposed?

I wanted to do what LZ4F_compressFrame() does but with the context allocated by us and for that I needed to call LZ4F_compressFrame_usingCDict which is not on the public API of the lib. Ofc there is a proper way to do this and it should be simple since LZ4F has some nice code examples but I am currently focusing on the last hanging fruit which is a small failure on one of the replication tests in cache mode. I am not worried about the API but I am worried about correctness 😄

If I make the progress on that I will jump right back here and implement this properly as you suggested.

@kostasrim
Copy link
Contributor Author

btw, I've actually used lz4 in other universe, here is an example of how to do it properly with lz4 context:

https://github.com/romange/gaia/blob/master/file/lz4_file.cc#L34

I suggest to keep this PR pending for now, and I may be able to fix it some time next week.

yes yes I am aware!

@kostasrim
Copy link
Contributor Author

Also @romange romange/helio@32e8c4e#diff-506d7b602acab7325378ccdba56ac52ca342af726ffb37147eeeafa3a1d943d5R306 was not very helpful.

Unless I am missing something we print the diff before we terminate but if we blow out the stack before we reach there, we won't get anything meaningful

@adiholden
Copy link
Collaborator

btw, I've actually used lz4 in other universe, here is an example of how to do it properly with lz4 context:

https://github.com/romange/gaia/blob/master/file/lz4_file.cc#L34

I suggest to keep this PR pending for now, and I may be able to fix it some time next week.

The api that you use in gaia is for steam compression but actually we dont need this here as we have all the data in memory once we call compress and we create one flame and send the data.
We did actually use this api before in dragonfly and I changed to use the LZ4F_compressFrameBound api in this commit ae9e3c4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

investigate compression issues with big value serialization
3 participants