OffHeap RegisterSet support

[krummas]

This makes it possible to keep the register set off heap - we need it in Cassandra for long lived HLLP instances (we use it for overlap estimation between sstables) https://issues.apache.org/jira/browse/CASSANDRA-11035

[tea-dragon]

I'm sorry, but this needs quite a bit of work to be acceptable. To start with, why didn't you try to just replace the int array with IntBuffer? Your current offheap implementation easily allows memory corruption / segfaults.

There are other significant issues as well as minor things like copying existing code style (there is in at least one case an if statement that uses braces replaced with one omitting them).

[krummas]

I disagree, but if it makes you feel better, I will refactor it to use a direct byte buffer

What other "significant" issues are you talking about? Not very helpful.

[krummas]

Why the hostility? Have I done anything wrong except try to help out your project?

[tea-dragon]

It's not intended as hostility. I'm falling asleep midway through and my girlfriend is waiting to finish watching a movie. Just terseness. Your original response indicated a need for details.

[tea-dragon]

but just for reference, here are some things you did wrong:

• you misrepresented yourself just now. your intention is help cassandra
• you saw fit to propose dangerous, undocumented usages of unsafe into the library, and then when the maintainer of the library made an objection, you could not resist the urge to blindly reassert perfection in an exceedingly rude manner
• you want to know more about other issues, in the middle of the night, when you'd just shown you couldn't handle the trivial issues originally mentioned?

The reason it isn't helpful is because being helpful to that detail is time consuming and exhausting. So to summarize, the biggest thing you did wrong was submit code so awful that there is no way it is going to be accepted without either harming the library, or inordinate levels of effort on my part. Most likely to the extent of rewriting it entirely or wishing I had. However, since cassandra wants it, and enough people with the power to ask me to do things will care about that, that's going to happen... and at this point, really, already has.

I hope you understand that in your future endevours.

[krummas]

First, it is not the middle of the night, I live in Sweden

Second, I absolutely did not intend to be rude, maybe it was a language thing, I just accepted your comment and fixed the pull request the way you suggested.

[krummas]

The question about the other significant issues was just so that I could fix all the things at once and not have to wait.

[tea-dragon]

And it was my admiration for that sentiment that, despite my personal, local, situation, motivated me to highlight as many as I could before my brain completely failed.

I only mentioned the time add an explanation for the terse nature of some of the comments.

On Thu, Mar 10, 2016, 03:31 Marcus Eriksson notifications@github.com wrote:

The question about the other significant issues was just so that I could fix all the things at once and not have to wait.

— Reply to this email directly or view it on GitHub https://github.com/addthis/stream-lib/pull/103#issuecomment-194732511.

[krummas]

OK, sorry for any misunderstanding and I hope the new patch is more suited for inclusion

[abramsm]

@krummas

First, thank you for the PR, any contribution to this project is appreciated. We are very happy that stream-lib is a part of Cassandra and the idea of storing registers off-heap is a good one.

Second, Please allow me to personally apologize for the rudeness of the comments in this thread. That type of response is completely unacceptable and not something we condone. I am embarrassed and I hope you can accept this apology.

This is a big change and it will require some testing and analysis before we can merge in. Give us some time to work through the proposed changes and we'll get back to you when we have more questions or alternative approaches.

Thanks, Matt Abrams

[mspiegel]

Thanks for the pull request! Here are some initial questions:

• can you run some Cassandra benchmarks with this change in stream-lib to confirm the benefits of this change?
• I'll need to read through the IntBuffer source code and/or run some benchmarks to confirm that there is no performance regression for the common use case (on heap)

Thanks!

@krummas https://github.com/krummas

First, thank you for the PR, any contribution to this project is appreciated. We are very happy that stream-lib is a part of Cassandra and the idea of storing registers off-heap is a good one.

Second, Please allow me to personally apologize for the rudeness of the comments in this thread. That type of response is completely unacceptable and not something we condone. I am embarrassed and I hope you can accept this apology.

This is a big change and it will require some testing and analysis before we can merge in. Give us some time to work through the proposed changes and we'll get back to you when we have more questions or alternative approaches.

Thanks, Matt Abrams

— Reply to this email directly or view it on GitHub https://github.com/addthis/stream-lib/pull/103#issuecomment-194845536.

[krummas]

This is for a new feature in Cassandra - with our earlier usage of stream-lib we deserialize the registers from disk whenever we need them, with this new feature we will be using them a lot more and need to keep them in memory all the time.

I will try to do a benchmark - we will potentially have 1000+ long lived registers in memory (one per sstable) and do many merges, will try to get something done early next week or so.

[yuesong]

Hi Marcus,

Thanks for your patience while we review the PR. We are a big user of Cassandra ourselves, so we are very happy to do our part to support this feature. Here are some of my thoughts on the PR:

• We are reluctant to incorporate Unsafe lightly into the library for obvious reasons. Previous comments on certain code in the Unsafe revision have technical merit. Direct buffer is a feasible workaround in that the usage is safer.
• (Direct) Buffer, however, is slower than Java native array/Unsafe when it's used as an array and when the main usage pattern is random access by index. (It's worth noting I haven't done any benchmarking myself. It's based on information I see online) Currently the PR uses Buffer for both on-heap and off-heap implementation. It's a common use case for us to merge a large number of HLLP counters so this may be a performance degradation for us.
• In neither revision does the PR provide a good way for freeing off-heap memory. We can't rely on the finalize method to do this automatically. The client code that creates the off-heap RegisterSet should be responsible for releasing its memory, and there should be an explicit API for the client code to do that.
• A minor thing in the latest revision is the use of Buffer.flip. The code is correct functionally, but I think it'll be easier for developers who are not familiar with Buffer to read if we just loop over the elements when copying elements to/from an array. (I actually had to look at the buffer code and do some experiment myself to make sure it's doing the right thing)

With all that, here's my suggestion:

• Make RegisterSet an interface
• Create an ArrayRegisterSet implementation that uses Java native array (basically the original code). This will be the default RegisterSet that HyperLogLog and HLLP will use.

With these two changes, you have the option now to create custom RegisterSet implementations in your application. You can pass them to the HLL and HLLP constructors. In your case for Cassandra, for example, you can implement an OffHeapRegisterSet (using Unsafe or IntBuffer). It will have a destroy method where it frees its memory. Cassandra code will keep references to all the unsafe register sets and call their destroy methods at the appropriate time. This way the core of the library is not affected while you get the flexibility to do the off-heap customization.

Consider adding serialize/deserialize methods to the RegisterSet interface. That way we may not have to worry about changing the version number. That should make the code simpler.

Let me know what you think.

Yuesong

[krummas]

sounds good! will update the patch as soon as possible