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

#225 implement decompression from direct buffer to heap buffer #226

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

Conversation

richardstartin
Copy link

@richardstartin richardstartin commented Jan 11, 2019

Allows decompression from a direct buffer to a non direct buffer. Tested on Ubuntu 16.0.4/JDK8/GCC 7.3.0 only.

@richardstartin richardstartin force-pushed the decompress-direct-to-heap branch 2 times, most recently from 91373ca to 5adaab4 Compare January 11, 2019 21:41
@richardstartin richardstartin changed the title #255 implement decompression from direct buffer to heap buffer #225 implement decompression from direct buffer to heap buffer Jan 11, 2019
@richardstartin
Copy link
Author

@xerial have you had a chance to take a look at this one?

@xerial xerial self-requested a review January 16, 2019 19:33
@xerial
Copy link
Owner

xerial commented Jan 16, 2019

@richardstartin Thanks for the PR.

I'm OK with adding this functionality, but this change will require re-compilation of all native libraries, including linux/ppc64, AIX, s390x. The other native libraries can be built by using cross compilers in docker images.

So a new version might not be available soon even if this PR is merged.

@richardstartin
Copy link
Author

@xerial thanks for the feedback. While this is the minimal feature I need, it sounds like the release process is onerous when native implementations are added. Perhaps it is worth implementing the cross product of byte[]/DirectByteBuffer/compress/uncompress shims first?

@xerial
Copy link
Owner

xerial commented Jan 16, 2019

@richardstartin I think so. For now compress(ByteBuffer, ByteBuffer) and uncompress(ByteBuffer, ByteBuffer) should be the only target to minimize the change.

Internally, we need to distinguish two types of buffers: heap ByteBuffer or off-heap ByteBuffer = DirectByteBuffer.

2 buffer types x 2 (src or dest) x 2 methods = 8 methods will be necessary to support all combinations.

I would like to minimize the number of such functions by passing buffer types to native method arguments, e.g.,

native int rawCompress(ByteBuffer src, int srcBufferType, ByteBuffer dest, int destBufferType)

@richardstartin
Copy link
Author

@xerial since it won’t be possible to get the functionality in this PR released quickly and I’m not in a position to maintain a fork, I am willing to implement the full feature set along these lines within the next couple of weeks.

If this larger implementation ends up being acceptable, what kind of timescale would we be looking at to get a release out, given the complexity of building the native artefacts? I ask for planning purposes rather than to be pushy - thanks for making this library available and maintaining it.

@xerial
Copy link
Owner

xerial commented Jan 16, 2019

@richardstartin Usually the PR review of this change size will need a couple of days (so expect one week after submitting PR), and releasing a preview version (e.g., snappy-java-1.1.8-p1 with some binaries missing for some OSs) can be done in an hour. I think this would work for your use case (e.g., using snappy-java only in Intel Linux machines)

More importantly just keep in touch with me using Twitter (@Taroleo).

@richardstartin
Copy link
Author

Hi @xerial sorry for the PR necromancy but what do you think about merging this as is (assuming a rebase)? I'm using snappy-java again all these years later and not being able to decompress from direct to heap is already a problem again.

if (uncompressed.isDirect()) {
decompressedSize = impl.rawUncompress(compressed, cPos, cLen, uncompressed, uncompressed.position());
} else {
decompressedSize = impl.rawUncompressDirectToHeap(compressed, cPos, cLen,
Copy link
Owner

Choose a reason for hiding this comment

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

We need some protection so as not to use this path if the native library is not build with this new method (e.g., AIX)

@xerial
Copy link
Owner

xerial commented Oct 28, 2021

@richardstartin Let me take a look again once the conflict is resolved.

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