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

Update c_src files to latest from HdrHistogram_c #32

Open
jamesvl opened this issue Jul 5, 2017 · 6 comments
Open

Update c_src files to latest from HdrHistogram_c #32

jamesvl opened this issue Jul 5, 2017 · 6 comments

Comments

@jamesvl
Copy link

jamesvl commented Jul 5, 2017

Would there be any interest in updating this project with the latest source files from the C version?

I've done the work in my working branch, but it comes with a number of significant caveats:

  • the latest version has removed (or renamed) a number of Histogram struct variables, so this would be a break change
  • the performance seems to be about 33% slower - running tests on my system slows down the operations from 10ns/h to ~13.5ns/h.
  • it removes the uncompressed option (and tests), since those don't seem to be present in the C version any more

If there's interest in an update like this I'll open a PR -- I'm interested in the update in case the C version has fixed any bugs that this project would want to pick up.

Obviously, if maintaining perfect backwards compatibility and the same level of performance are more important, we can close this Issue out. Thoughts?

@darach
Copy link
Contributor

darach commented Jul 6, 2017

Hi James,

This has been on my TODO list for a while. The uncompressed option was added by @Licenser as he uses compressed filesystems so didn't need compression. We'd probably need to keep that as an extension.

I'll take a look at your branch over the weekend.

Not sure native binary compatibility is a major issue as long as the erlang side is binary compatible.

Cheers,

Darach.

@jamesvl
Copy link
Author

jamesvl commented Jul 6, 2017

Derach -

That sounds great. I may need some help getting the uncompressed stuff to work again, but I'll dig around in the C source more and see if I can get it working. Let me know if you see any other issues when you get a chance to check...

@jamesvl
Copy link
Author

jamesvl commented Jul 11, 2017

I did take a look at adding the uncompressed writer back in - unfortunately, there's no code path for that anymore in the C version - all write methods will use gzip + base64 encoding, per the Java version. Any suggestions on how to proceed?

@mikeb01
Copy link

mikeb01 commented Jul 12, 2017

@darach How important is uncompressed encode/decode? I can add it back in if required.

@tsloughter
Copy link
Collaborator

I think we should update hdrhistogram version. @jamesvl did you have any work on this?

If latest hdrhistogram does not support uncompressed then we just have to drop that.

@Licenser would it hurt you that much if only compressed was suported?

@tsloughter
Copy link
Collaborator

And I just noticed that neither the C or Rust versions support doubles... Hm.

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

No branches or pull requests

4 participants