lz4 / lz4-java

LZ4 compression for Java
Apache License 2.0
1.1k stars 253 forks source link

Using Java unsafe implementation crashes JVM on Solaris sparcv9 with 64 bit JVM #33

Closed Shohou closed 10 years ago

Shohou commented 10 years ago

Hi, Using lz4 under solaris sparcv9 with 64bit jvm is not possible at the moment. If I use LZ4Factory.fastestInstance() and factory.fastCompressor() it tries to use Unsafe and once it does so, JVM crashes with problematic frame [libjvm.so+0xc5212c] Unsafe_GetInt+0x158. I looked at the java DirectByteBuffer and saw that Java checks for architecrure before using Unsafe and if it is not one of the known, it reads ints and longs by bytes. This commit changes UnsafeUtils to check os.arch and if it is not one of the known, it reads ins and long byte by byte. Running under 32 bit java on solaris passes all tests. Running under 64 bit java passes all tests except those that use native library, for some reason native library doesn't work for me. It crashes at some point. Maybe I just dont know how to compile it.

Shohou commented 10 years ago

This checks for every read will defenitely add some overhead for unsafe implementation, meybe it's better to check for allowed unaligned reads when deciding which implementation to use, and if unaligned reads are not allowed then always return safe java implementation. It's up to you to decide which is better.

jpountz commented 10 years ago

Thanks for reporting this issue!

meybe it's better to check for allowed unaligned reads when deciding which implementation to use, and if unaligned reads are not allowed then always return safe java implementation

I like this idea better. The unsafe impl is based on the assumption that reading ints or longs is very cheap. For example in LZ4UnsafeUtils.commonBytes, it only reads longs to find the largest prefix length between two sequences. If the method to read longs happened to read byte-by-byte, it would probably be better to compare bytes like in LZ4Utils.commonBytes.

If you could provide a pull request for this idea, that would be great!

Shohou commented 10 years ago

sure, I will make another pull

Shohou commented 10 years ago

I changed commit, please review