You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by davisp <gi...@git.apache.org> on 2018/10/16 16:10:49 UTC
[GitHub] couchdb-snappy pull request #9: Fix memory bug in SnappyNifSink::Append
GitHub user davisp opened a pull request:
https://github.com/apache/couchdb-snappy/pull/9
Fix memory bug in SnappyNifSink::Append
Previously `SnappyNifSink` assumed that `GetAppendBuffer` was always
called before `Append`. This turned out to be an invalid assumption.
This was definitely in the land of "How did that even work?". The simple
fix is simple.
This also pre-allocates the write buffer to 8192 bytes which just saves
us from the initial re-allocation on first `Append` since we allocated a
zero length buffer initially.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-snappy fix-sink-buffer
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-snappy/pull/9.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9
----
commit 60572a1a4a9fe3cf4e97ccf14df100080481791a
Author: Paul J. Davis <pa...@...>
Date: 2018-10-16T15:51:18Z
Normalize capitalization
commit 2038ad13b1d6926468f25adea110028e3c0b4b0c
Author: Paul J. Davis <pa...@...>
Date: 2018-10-16T16:07:58Z
Fix memory bug in SnappyNifSink::Append
Previously `SnappyNifSink` assumed that `GetAppendBuffer` was always
called before `Append`. This turned out to be an invalid assumption.
This was definitely in the land of "How did that even work?". The simple
fix is simple.
This also pre-allocates the write buffer to 8192 bytes which just saves
us from the initial re-allocation on first `Append` since we allocated a
zero length buffer initially.
----
---
[GitHub] couchdb-snappy issue #9: Fix memory bug in SnappyNifSink::Append
Posted by nickva <gi...@git.apache.org>.
Github user nickva commented on the issue:
https://github.com/apache/couchdb-snappy/pull/9
To check for memory leaks ran the snappy compression decompression test in a loop
```
rebar shell
==> couchdb-snappy (shell)
Erlang/OTP 20 [erts-9.3.3.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V9.3.3.1 (abort with ^G)
1> cd("test").
2> c(snappy_tests).
3> [begin snappy_tests:test(), timer:sleep(10) end || _ <- lists:seq(1,10000)].
...
```
Memory stayed at 40+/2 Mb
---
[GitHub] couchdb-snappy pull request #9: Fix memory bug in SnappyNifSink::Append
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-snappy/pull/9
---
[GitHub] couchdb-snappy pull request #9: Fix memory bug in SnappyNifSink::Append
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on a diff in the pull request:
https://github.com/apache/couchdb-snappy/pull/9#discussion_r225681578
--- Diff: c_src/snappy_nif.cc ---
@@ -100,6 +94,21 @@ SnappyNifSink::getBin()
}
+void
+SnappyNifSink::EnsureSize(size_t append_length)
+{
+ if((length + append_length) > bin.size) {
+ size_t sz = append_length * 4;
--- End diff --
I have no idea why 4 was picked originally but that's not an architecture thing I don't think? Its just pre-allocating more space than required so that we aren't reallocating for every single append operation. That could be 3 or 5 or 8,000,000.
---
[GitHub] couchdb-snappy pull request #9: Fix memory bug in SnappyNifSink::Append
Posted by wohali <gi...@git.apache.org>.
Github user wohali commented on a diff in the pull request:
https://github.com/apache/couchdb-snappy/pull/9#discussion_r225660319
--- Diff: c_src/snappy_nif.cc ---
@@ -100,6 +94,21 @@ SnappyNifSink::getBin()
}
+void
+SnappyNifSink::EnsureSize(size_t append_length)
+{
+ if((length + append_length) > bin.size) {
+ size_t sz = append_length * 4;
--- End diff --
why not use `sizeof()` here, rather than a fixed 4 bytes, since we target multiple architectures and people are beating down my door for proper arm32/arm64/ppc64le support?
---