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

Use cramjam and py3 speed for string decode #580

Merged
merged 6 commits into from
Apr 19, 2021

Conversation

martindurant
Copy link
Member

@milesgranger , this just touches the top cramjam functions. The best speedup would come from deciding when decompress_into can be used (since the target arrays are always pre-allocated). As it stands, I don't expect much speed difference compared to main branch.

Also, I took the chance to finally update UTF8 reading, doing the conversion to python strings in one step instead of reading bytes and then converting. Gives a 2x speedup.

@milesgranger
Copy link

Nice! 🚀

I suppose the README may need a small updated for available algorithms and which are included as standard?

Also, I don't know if you'll hit it in the future, but cramjam.snappy.compress_into may raise an error for your scenario because the underlying snap crate, checks the destination buffer for at least max_compress_len so if the output is known but is still smaller than what snappy thinks it needs, it would fail.

Anyway, if that happens I think it's a good use case for supporting something like a compress_unchecked type API in the snap crate; then cramjam could just switch to that.

@martindurant
Copy link
Member Author

it's a good use case for supporting something like a compress_unchecked (_into)

Also slightly faster, I imagine.

I have not yet decided whether this PR should press further to get the uncompress_into benefit or to keep it simple at first. I'll update docs when merging.

@martindurant
Copy link
Member Author

OK, It will take more time than I have right now to plumb in correct use of decompress_into.
One thing to check here before merger: I notice that lz4 has a "raw" option too, so need to check whether the function called now in the code is actually the same as the one provided by cramjam - so can swap that one out too.

@milesgranger
Copy link

milesgranger commented Apr 12, 2021

If you're using "raw" (assuming this is synonymous with "block") in fastparquet, then cramjam would need an update to support that. Currently it uses framed format by default, but raw/block can be exposed the same way raw is for snappy.
Ref: https://bozaro.github.io/lz4-rs/lz4/block/index.html

@martindurant
Copy link
Member Author

I expect you are correct - but it should take just ten minutes of my time, whenever I can find it, to check.

@martindurant
Copy link
Member Author

Yep, it seems that fastparquet and cramjam don't use the same lz4, which must mean the former is the "raw" variant.

In [3]: data = b'hello you'

In [4]: out = fastparquet.compression.compressions['LZ4'](data)

In [5]: import cramjam

In [6]: cramjam.lz4.decompress(out)
---------------------------------------------------------------------------
DecompressionError                        Traceback (most recent call last)
<ipython-input-6-ee7796129fd7> in <module>
----> 1 cramjam.lz4.decompress(out)

DecompressionError: LZ4 error: ERROR_frameType_unknown

Fastparquet is calling lz4.block.compress

@martindurant
Copy link
Member Author

Also, compression passes store_size=False, so this would be "no prefix"

@milesgranger
Copy link

Nice, thanks for the info. I'll try to have a PR to cramjam later this evening or tomorrow with the block format for lz4.

Martin Durant added 3 commits April 19, 2021 09:34
This used to be the done thing ... before wheels! Now it's just bloat.
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.

2 participants