Use Ejson Body Instead of Compressed Body for External size #606

tonysun83 · 2017-06-21T16:55:32Z

Overview

In two places where we calculate the ExternalSize of the document body,
we use the Summary which is a compressed version of the doc body. We
change this to use the actual ejson body. In copy_docs we don't have
access to the #doc record so we can't access the meta where we store
the ejson body. Unfortunately, this means we have to decompress the
document body after reading it from disk.

Testing recommendations

Performance tests were conducted to compare pre-fix External Size values and post-fix External Size Values. Here are the results:

Version        Compression    DB Dize      DB External Size

Pre-Fix         snappy            70 MB        46249702       
Pre-Fix         snappy            700 MB      462495533       
Pre-Fix         snappy            2.1 GB        1387486406      
                                        
Post-Fix       snappy            70 MB        83950133    
Post-Fix       snappy            700 MB      839500133        
Post-Fix       snappy            2.1 GB        2518500133         
                                        
Pre-Fix         deflate_6        70 MB        46249702         
Pre-Fix         deflate_6        700 MB      413395945      
Pre-Fix         deflate_6        2.1 GB       994598087         
                                        
Post-Fix       deflate_6        70 MB        83950133   
Post-Fix       deflate_6        700 MB      430495496  
Post-Fix       deflate_6        2.1 GB       1291486342

GitHub issue number

PR is number.

Related Pull Requests

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;

In two places where we calculate the ExternalSize of the document body, we use the Summary which is a compressed version of the doc body. We change this to use the actual ejson body. In copy_docs we don't have access to the #doc record so we can't access the meta where we store the ejson body. Unfortunately, this means we have to decompress the document body after reading it from disk. COUCHDB-3429

davisp · 2017-06-23T17:23:24Z

Not sure what your test was there, but that's a pretty big red flag that the external sizes don't match between different compression algorithms as it should not be affected by that. You also probably want to do a pre/post compaction measurement as well to make sure compaction isn't affecting it either.

davisp

Looks pretty close but we'll want to figure out why the snappy/deflate numbers don't agree. You'll probably also want to add a no-compression check as well. External should be the same for all of those and both before and after compaction.

Also for this test you should probably look at the node local HTTP port for testing so that you're not getting effects from a clustered database.

davisp · 2017-06-23T17:24:13Z

src/couch/src/couch_db.erl

- Doc#doc{body = {summary, SummaryChunk, SizeInfo, AttsFd}}
+ Meta = Doc#doc.meta,
+ Doc#doc{body = {summary, SummaryChunk, SizeInfo, AttsFd},
+ meta = [{ejson_body, Body} | Meta]}


You should just store the size of the body here rather than pass the whole body. By doing that you're effectively nullifying the summary optimization of only passing binaries in and out of couch_db_updater.

davisp · 2017-06-23T17:26:59Z

src/couch/src/couch_db_updater.erl

@@ -1086,8 +1088,15 @@ copy_docs(Db, #db{fd = DestFd} = NewDb, MixedInfos, Retry) ->
 {NewRevTree, FinalAcc} = couch_key_tree:mapfold(fun
 (_Rev, #leaf{ptr=Sp}=Leaf, leaf, SizesAcc) ->
 {Body, AttInfos} = copy_doc_attachments(Db, Sp, DestFd),
+ IsComp = couch_compress:is_compressed(Body, Compress),
+ EJsonBody = case IsComp of


This check isn't quite right as we might have changed the compression and we're looking at something that's not been compressed with the same method. Just use is_binary and then pass it to couch_compress:decompress if so.

Ah, though I see that's based off the make_doc_summary code. The difference here is that we don't care what its compressed as, where as make_doc_summary is converting a possibly compressed document to a different compression scheme.

COUCHDB-3429

couchdb-3429

tonysun83 · 2017-06-27T05:01:52Z

@davisp: I made the changes you suggested. Also, I think the performance benchmarks were using port 5984 instead of 5986, so it was influenced by the clustered nature of things. Compression tests existed, so I just piggy backed off that code. Let me know if this looks good, and I'll create a temporary build to check the performance numbers before merging.

Couchdb-3429

couchdb-3429

davisp

Couple minor changes but it looks mostly good. Only real concern is the decompress call in the compactor.

davisp · 2017-07-12T17:25:03Z

src/couch/src/couch_db.erl

- Doc#doc{body = {summary, SummaryChunk, SizeInfo, AttsFd}}
+ Meta = Doc#doc.meta,
+ ExternalSize = ?term_size(Body),
+ Doc#doc{body = {summary, SummaryChunk, SizeInfo, AttsFd},


Move attributes to their own lines when you wrap records. Something like:

Doc#doc{ body = {summary, ...}, meta = [{ejson_size, ?term_size(Body)} | Meta] }

davisp · 2017-07-12T17:27:28Z

src/couch/src/couch_db_updater.erl

@@ -1076,6 +1076,7 @@ check_md5(_, _) -> throw(md5_mismatch).

 copy_docs(Db, #db{fd = DestFd} = NewDb, MixedInfos, Retry) ->
 DocInfoIds = [Id || #doc_info{id=Id} <- MixedInfos],
+ Compress = NewDb#db.compression,


Not seeing where this is used.

davisp · 2017-07-12T17:28:31Z

src/couch/src/couch_db_updater.erl

+ {ejson_size, ExternalSize} ->
+ ExternalSize;
+ false ->
+ couch_compress:decompress(Summary)


This is a bug right? You should be doing ?term_size(couch_compress:decompress(Summary)).

I call ExternalSize = ?term_size(EJsonBody), further down

Then your ejson_size you calculated is going to reduce all doc bodies to 4 bytes or whatever since you're returning an integer?

Also looking at the code I don't see where you call it.

wups, I was looking at the other place

davisp · 2017-07-12T17:30:34Z

src/couch/src/couch_db_updater.erl

@@ -1086,8 +1087,14 @@ copy_docs(Db, #db{fd = DestFd} = NewDb, MixedInfos, Retry) ->
 {NewRevTree, FinalAcc} = couch_key_tree:mapfold(fun
 (_Rev, #leaf{ptr=Sp}=Leaf, leaf, SizesAcc) ->
 {Body, AttInfos} = copy_doc_attachments(Db, Sp, DestFd),
+ EJsonBody = case is_binary(Body) of


I see why we need to do this for existing shards that may have an incorrect value however I'm not a huge fan of this being a permanent change and not just around for an upgrade. Would be nice if we could figure out how to special case this so it only runs the decompression when we need to upgrade.

davisp

+1

…606)" This reverts commit dce6e34.

…l size (#606)"" This reverts commit c8ee295. PR is here: #606

Use ejson body instead of compressed body for external size In two places where we calculate the ExternalSize of the document body, we use the Summary which is a compressed version of the doc body. We change this to use the actual ejson body. In copy_docs we don't have access to the #doc record so we can't access the meta where we store the ejson body. Unfortunately, this means we have to decompress the document body after reading it from disk. COUCHDB-3429

tonysun83 mentioned this pull request Jun 21, 2017

Use ejson body instead of compressed body for external size cloudant/couchdb#4

Closed

3 tasks

davisp requested changes Jun 23, 2017

View reviewed changes

Tony Sun added 2 commits June 26, 2017 18:34

Use size in meta instead of Body

c2e2f23

COUCHDB-3429

use is_binary

64d848c

couchdb-3429

Tony Sun added 3 commits June 26, 2017 22:03

Add test to make for different compression methods

7fec381

Couchdb-3429

add pre-compaction external size value

a8b2200

couchdb-3429

whitespace

8b82557

davisp requested changes Jul 12, 2017

View reviewed changes

Tony Sun added 2 commits July 12, 2017 10:56

cleanup and formatting

b3eb718

add comment

c07d437

davisp approved these changes Jul 14, 2017

View reviewed changes

fix missing term_size

2065100

tonysun83 merged commit dce6e34 into master Jul 15, 2017

tonysun83 deleted the 3429-external-size-dbs branch July 15, 2017 17:27

rnewson added a commit that referenced this pull request Jul 15, 2017

Revert "Use Ejson Body Instead of Compressed Body for External size (#…

c8ee295

…606)" This reverts commit dce6e34.

rnewson added a commit that referenced this pull request Jul 15, 2017

Revert "Revert "Use Ejson Body Instead of Compressed Body for Externa…

8506afc

…l size (#606)"" This reverts commit c8ee295. PR is here: #606

nickva pushed a commit to nickva/couchdb that referenced this pull request Sep 7, 2022

Link to CouchDB wiki SSL walkthru, closes apache#606 (apache#647)

f262e01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Ejson Body Instead of Compressed Body for External size #606

Use Ejson Body Instead of Compressed Body for External size #606

tonysun83 commented Jun 21, 2017 •

edited

Loading

davisp commented Jun 23, 2017

davisp left a comment

davisp Jun 23, 2017

davisp Jun 23, 2017

davisp Jun 23, 2017

tonysun83 commented Jun 27, 2017 •

edited

Loading

davisp left a comment

davisp Jul 12, 2017

davisp Jul 12, 2017

davisp Jul 12, 2017

tonysun83 Jul 12, 2017

davisp Jul 12, 2017

davisp Jul 12, 2017

tonysun83 Jul 14, 2017

davisp Jul 12, 2017

davisp left a comment

Use Ejson Body Instead of Compressed Body for External size #606

Use Ejson Body Instead of Compressed Body for External size #606

Conversation

tonysun83 commented Jun 21, 2017 • edited Loading

Overview

Testing recommendations

GitHub issue number

Related Pull Requests

Checklist

davisp commented Jun 23, 2017

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonysun83 commented Jun 27, 2017 • edited Loading

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

tonysun83 commented Jun 21, 2017 •

edited

Loading

tonysun83 commented Jun 27, 2017 •

edited

Loading