Mv level work1 #73

Merged
merged 4 commits into from Mar 14, 2013

Conversation

Projects
None yet
2 participants
Contributor

matthewvon commented Mar 11, 2013

I took a series of constants and put them into a static structure such that I can vary the constants by level (instead of one set of constants for all levels). Then I found all places in this file that had special "if (level==0)" code and changed it to "if (m_LevelTraits[level].m_OverlappedFiles==true)". Finally, some code relating to the m_MaxGrandParentOverlapBytes constant was disabled.

Testing shows a 48% increase in write throughput. Slight increases in read and iterate throughput, but not worth mentioning.

@ghost ghost assigned matthewvon and engelsanchez Mar 11, 2013

A little stuttering typo here

db/version_set.cc
+// write buffer size of 60,000,000. Why five times: 4 level-0 files typically compact
+// to one level-1 file and are each slightly larger than 60,000,000.
+// level-1 file size of 1,500,000,000 applies to output file of this level
+// being writtne to level-2. The value is five times the 300,000,000 of level-1.
@engelsanchez

engelsanchez Mar 11, 2013

Little typo "writtne" here

+ int level;
+
+ for (level=0; level < config::kNumLevels; ++level)
+ {
@engelsanchez

engelsanchez Mar 11, 2013

More annoying nitpicks. These files seem to stick to K&R initial bracket placements. It might be good to clean up if this will ever merge upstream.

@matthewvon

matthewvon Mar 14, 2013

Contributor

No short term or long term intention to merge. Going with style that opens up the code for easier review / modification, i.e. switching from original coder focus to maintenance coder focus.

Maybe update the comment here? Stale comments kill people.

Update comment here too?

Ok then. So the big change here, to summarize, is we allow overlapping ranges in levels 0-2 instead of just 0. Now compactions take into account moving up a level that allows overlapping ranges and will not merge any files from that level+1, hence possibly creating an overlapping file from the result of the level files. And these constants reflect tuning that you have done and allow you to keep fine tuning for each level. Also, we're now dumping memtables to level 0 only, since the optimization to figure out if it could go up some levels has been removed in favor of the above. Does that sound like a good summary?

It all sounds good to me and I can't find any problems in the code besides the comment/style nitpicks. I can't say I can verify the performance benchmarks. We'd need to clone @matthewvon for that :).

If you could elaborate a bit on why removing the trivial move optimization is OK, that would satisfy my curiosity.

But consider this a 👍 💃 already.

matthewvon pushed a commit that referenced this pull request Mar 14, 2013

@matthewvon matthewvon merged commit 05c900f into master Mar 14, 2013

@ghost ghost assigned matthewvon Mar 14, 2013

@matthewvon matthewvon deleted the mv-level-work1 branch May 5, 2016

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