Skip to content

Conversation

@ghatdev
Copy link
Contributor

@ghatdev ghatdev commented Jan 25, 2018

No description provided.

@Viq111
Copy link
Collaborator

Viq111 commented Jan 25, 2018

Hi @ghatdev thanks for your PR!
Sorry I haven't been up to date on that repo but I just pushed the integration with Travis to run automatically the tests on PRs, would you mind pulling the latest changes from 1.x branch to run those tests ?

@ghatdev
Copy link
Contributor Author

ghatdev commented Jan 26, 2018

oops I missed it. I just did!
there are some errors with go test manually:

--- FAIL: TestZstdReaderLong (0.00s)
zstd_stream_test.go:24: Compressed 130000 -> 33 bytes
panic: runtime error: slice bounds out of range [recovered]
panic: runtime error: slice bounds out of range

goroutine 8 [running]:
testing.tRunner.func1(0xc42009a3c0)
/usr/local/Cellar/go/1.9.2/libexec/src/testing/testing.go:711 +0x2d2
panic(0x41a0540, 0x4275be0)
/usr/local/Cellar/go/1.9.2/libexec/src/runtime/panic.go:491 +0x283
github.com/ghatdev/go-zstd.Decompress.func1(0xc420062070, 0x63, 0x63, 0xc4200a4258, 0x21, 0x40, 0x0, 0xc420014340, 0xc420045cf8, 0x40b7405, ...)
/Users/ghatdev/Work/Go/src/github.com/ghatdev/go-zstd/zstd.go:145 +0x184
github.com/ghatdev/go-zstd.Decompress(0x0, 0x63, 0x0, 0xc4200a4258, 0x21, 0x40, 0x0, 0x0, 0x0, 0x0, ...)
/Users/ghatdev/Work/Go/src/github.com/ghatdev/go-zstd/zstd.go:163 +0x129
github.com/ghatdev/go-zstd.testCompressionDecompression(0xc42009a3c0, 0x0, 0x0, 0x0, 0xc42016a000, 0x1fbd0, 0x267f3)
/Users/ghatdev/Work/Go/src/github.com/ghatdev/go-zstd/zstd_stream_test.go:28 +0x37f
github.com/ghatdev/go-zstd.TestZstdReaderLong(0xc42009a3c0)
/Users/ghatdev/Work/Go/src/github.com/ghatdev/go-zstd/zstd_stream_test.go:89 +0x10c
testing.tRunner(0xc42009a3c0, 0x41cd4d8)
/usr/local/Cellar/go/1.9.2/libexec/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
/usr/local/Cellar/go/1.9.2/libexec/src/testing/testing.go:789 +0x2de
exit status 2
FAIL github.com/ghatdev/go-zstd 0.015s

@ghatdev
Copy link
Contributor Author

ghatdev commented Jan 26, 2018

I checked compress, decompress function only but they worked.

@ghatdev
Copy link
Contributor Author

ghatdev commented Jan 26, 2018

it seems to be some changes on ZSTD_decompress
zstd.go -> Decompress:written should be -13 but 1.3.3 headers return -70

@develar
Copy link

develar commented Mar 7, 2018

What's status of this PR?

@Viq111
Copy link
Collaborator

Viq111 commented Mar 7, 2018

Hi, I haven't looked at updating it yet myself, I'll have more time the next upcoming week.
From what I see currently, is that there are actually some changes to make to the wrapper to be able to use 1.3.3

@develar
Copy link

develar commented Mar 7, 2018

Thanks for status update. I use this zstd bindings for a tool creating tar.zstd files (https://github.com/develar/zstd-archiver) and it works great, but 1.3.3 brings multirthreads and long range matching.

@Viq111
Copy link
Collaborator

Viq111 commented Mar 14, 2018

Hi @ghatdev , I was able to take some time this morning to debug your PR.

I added a PR to fix (== completly get rid of) the static mapping of error code between Go and C: #29

I also pulled your branch and was able to make everything pass: #28

Basically:

Once this is done, I can merge that PR 👍

@ghatdev
Copy link
Contributor Author

ghatdev commented Mar 15, 2018

@Viq111 fc88a9d commit seems to be deleted from your test-1.3.3 branch in #28

@Viq111
Copy link
Collaborator

Viq111 commented Mar 15, 2018

Ah github might be too aggressive when pruning loose objects (using now instead of 30 days).

I re-pushed to 1697446

I think in your 5c04739 you missed 128 << 10 for 128 * 10

(Build should be passing here: https://travis-ci.org/DataDog/zstd/builds/353765142)

@Viq111
Copy link
Collaborator

Viq111 commented Mar 15, 2018

Great thanks a lot for your contribution! Looks good now and tests are passing.
Would you mind rebasing the history to have it a bit cleaner ?

@ghatdev
Copy link
Contributor Author

ghatdev commented Mar 15, 2018

@Viq111 I'm not good at using git :(.. Sorry about it.
Maybe I need some hints to do that.

@develar
Copy link

develar commented Mar 15, 2018

rebasing the history to have it a bit cleaner ?

@Viq111 Hint — you can merge this PR using "Squash & merge" to have one rebased commit.

@ghatdev ghatdev force-pushed the 1.x branch 2 times, most recently from 4f81c11 to 9269a8a Compare March 15, 2018 13:28
We have this function in Go code to not have to do a cgo call (really consuming). This function in a hot loop, that's why we can't call C code every time.

This mirrors latest zstd implementation (they switched to 128kB window)
@ghatdev
Copy link
Contributor Author

ghatdev commented Mar 15, 2018

@develar Thx!
@Viq111 combined to 1 commit finally. Is this fine?

Copy link
Collaborator

@Viq111 Viq111 left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks a lot for the PR!

@Viq111 Viq111 merged commit ca64c66 into DataDog:1.x Mar 15, 2018
@Viq111
Copy link
Collaborator

Viq111 commented Mar 15, 2018

@develar I usually don't like squashing because you loose git history (why a change was made).
Here, having a commit to update the C code and a separate commit for the CompressBound make it easy to understand what changed (and keep the commit message with an explanation)

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.

3 participants