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

Avoid decompressing just to calculate external size #835

Merged
merged 1 commit into from Sep 25, 2017

Conversation

Projects
None yet
4 participants
@nickva
Contributor

nickva commented Sep 22, 2017

Use snappy's uncompressed_length and external binary format's binary spec to
get uncompressed size.

http://erlang.org/doc/apps/erts/erl_ext_dist.html

@nickva nickva changed the title from Avoid uncompressing document bodies just to calculate external size to Avoid decompressing just to calculate external size Sep 22, 2017

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Sep 22, 2017

Member

Looks good to me, but changing ?term_size/1 scares me. @davisp - second opinion, please?

Member

eiri commented Sep 22, 2017

Looks good to me, but changing ?term_size/1 scares me. @davisp - second opinion, please?

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Sep 22, 2017

Member

Also addresses #829, for sake of references.

Member

eiri commented Sep 22, 2017

Also addresses #829, for sake of references.

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Sep 22, 2017

Member

I’ve independently and accidentally developed nearly the same patch. I avoided redefining ?term_size, but this lgtm if @davisp agrees.

Would be nice to get a few more numbers on how much faster this is cc @chewbranca. This could play a nice role in the 2.2 announcement.

Member

janl commented Sep 22, 2017

I’ve independently and accidentally developed nearly the same patch. I avoided redefining ?term_size, but this lgtm if @davisp agrees.

Would be nice to get a few more numbers on how much faster this is cc @chewbranca. This could play a nice role in the 2.2 announcement.

false ->
Body
?term_size(Body)

This comment has been minimized.

@janl

janl Sep 22, 2017

Member

If you move ?term_size() into couch_compress:uncompressed_size(), then you can get rid of the case and make this ExternalSize = couch_compress:uncompressed_size(Body), (at least in my limited testing, you may be covering for cases I haven’t seen)

@janl

janl Sep 22, 2017

Member

If you move ?term_size() into couch_compress:uncompressed_size(), then you can get rid of the case and make this ExternalSize = couch_compress:uncompressed_size(Body), (at least in my limited testing, you may be covering for cases I haven’t seen)

This comment has been minimized.

@nickva

nickva Sep 22, 2017

Contributor

@janl thought of that but noticed how in decompress there is an error for anything that's not an external term or snappy compression, so thought of keeping the same structure.

@nickva

nickva Sep 22, 2017

Contributor

@janl thought of that but noticed how in decompress there is an error for anything that's not an external term or snappy compression, so thought of keeping the same structure.

Avoid decompressing just to calculate external size
Use snappy's `uncompressed_length` and external binary format's binary spec to
get uncompressed size.

http://erlang.org/doc/apps/erts/erl_ext_dist.html

`erlang:external_size` is function provided since R16B3 use it without the
`try ... catch` fallback. Also make sure to use `[{minor_version, 1}]` to match
what `?term_to_bin` macro does.

Fixes #835
@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Sep 22, 2017

Member

Ran more numbers, a 1GB database (npm registry subset) with the whole database file in the fs cache (e.g. no read activity on the disk), seeing improvements between 10% and 30% in both CPU and wall clock time (normalised over 3 runs each), and corresponding higher write throughput to disk. All on my laptop so highly unscientific, but enough of a difference to mark as significant.

Member

janl commented Sep 22, 2017

Ran more numbers, a 1GB database (npm registry subset) with the whole database file in the fs cache (e.g. no read activity on the disk), seeing improvements between 10% and 30% in both CPU and wall clock time (normalised over 3 runs each), and corresponding higher write throughput to disk. All on my laptop so highly unscientific, but enough of a difference to mark as significant.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Sep 22, 2017

Contributor

Good call on benchmarking this.

I have a compaction benchmark script and with deflate_9 compression and large docs it shows good improvement about 40% or so.

https://gist.github.com/nickva/e87aa2f7f896805bfee36a044a3800b0

So it makes 100k docs of size 10k each. Then times compaction.

master

$ ./compact_bench.py -n 100000 -s 10000

**************** num=100000,size=10000 ****************
 Updating   : 1059.3s docs/s:94 revs/s:94 fsize:850620615
 Compacting : 35.5s docs/s:2816 revs/s:2816 fsize:773591239

PR branch

$ ./compact_bench.py -n 100000 -s 10000

**************** num=100000,size=10000 ****************
 Updating   : 1000.7s docs/s:99 revs/s:99 fsize:850620615
 Compacting : 19.6s docs/s:5091 revs/s:5091 fsize:773599431
Contributor

nickva commented Sep 22, 2017

Good call on benchmarking this.

I have a compaction benchmark script and with deflate_9 compression and large docs it shows good improvement about 40% or so.

https://gist.github.com/nickva/e87aa2f7f896805bfee36a044a3800b0

So it makes 100k docs of size 10k each. Then times compaction.

master

$ ./compact_bench.py -n 100000 -s 10000

**************** num=100000,size=10000 ****************
 Updating   : 1059.3s docs/s:94 revs/s:94 fsize:850620615
 Compacting : 35.5s docs/s:2816 revs/s:2816 fsize:773591239

PR branch

$ ./compact_bench.py -n 100000 -s 10000

**************** num=100000,size=10000 ****************
 Updating   : 1000.7s docs/s:99 revs/s:99 fsize:850620615
 Compacting : 19.6s docs/s:5091 revs/s:5091 fsize:773599431
@davisp

This comment has been minimized.

Show comment
Hide comment
@davisp

davisp Sep 25, 2017

Member

@eiri Actually not having minor_version in there is a bit of an oversight as we use minor_version everywhere when doing term_to_binary calls before writing to disk (as we should).

+1

Great find!

Member

davisp commented Sep 25, 2017

@eiri Actually not having minor_version in there is a bit of an oversight as we use minor_version everywhere when doing term_to_binary calls before writing to disk (as we should).

+1

Great find!

@nickva nickva merged commit 8d1c704 into apache:master Sep 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nickva nickva deleted the cloudant:dont-uncompress-just-to-get-size branch Sep 25, 2017

wohali added a commit that referenced this pull request Oct 19, 2017

Avoid decompressing just to calculate external size
Use snappy's `uncompressed_length` and external binary format's binary spec to
get uncompressed size.

http://erlang.org/doc/apps/erts/erl_ext_dist.html

`erlang:external_size` is function provided since R16B3 use it without the
`try ... catch` fallback. Also make sure to use `[{minor_version, 1}]` to match
what `?term_to_bin` macro does.

Fixes #835

willholley added a commit to willholley/couchdb that referenced this pull request May 22, 2018

Avoid decompressing just to calculate external size
Use snappy's `uncompressed_length` and external binary format's binary spec to
get uncompressed size.

http://erlang.org/doc/apps/erts/erl_ext_dist.html

`erlang:external_size` is function provided since R16B3 use it without the
`try ... catch` fallback. Also make sure to use `[{minor_version, 1}]` to match
what `?term_to_bin` macro does.

Fixes #835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment