search

dermesser / leveldb-rs

A reimplementation of LevelDB in Rust (no bindings).
Other
521 stars 59 forks source link

Do compactions after writing memtables (#34) #36

Closed dermesser closed 1 year ago

dermesser commented 1 year ago

@Sword-Smith I managed to reproduce the behavior you saw, and agree that it is untenable. Writing more and more L0 tables for some reason doesn't trigger a compaction. Therefore I attempted a brute-force approach - this definitely helps in my experiment. Maybe you can beta-test it for a few days?

Sword-Smith commented 1 year ago

Will try ASAP. I just have one question: Does this really add compaction? When I read the code, I can't see that compaction is called anywhere now where it wasn't before.

dermesser commented 1 year ago

Will try ASAP. I just have one question: Does this really add compaction? When I read the code, I can't see that compaction is called anywhere now where it wasn't before.

That's the "elegant" part I mentioned in the second commit :-) https://github.com/dermesser/leveldb-rs/pull/36/files#diff-15ff201495d5228eb056866352f17ee31902aba97620252fe0813ee133adfcc9L598

By removing the else, a compaction may be triggered after writing an L0 file. This will result in a compaction if more than 4 L0 files are present. Before, more and more L0 files would be written but the database would never check if there is a need to compact the L0 files.

Sword-Smith commented 1 year ago

Will try ASAP. I just have one question: Does this really add compaction? When I read the code, I can't see that compaction is called anywhere now where it wasn't before.

That's the "elegant" part I mentioned in the second commit :-) https://github.com/dermesser/leveldb-rs/pull/36/files#diff-15ff201495d5228eb056866352f17ee31902aba97620252fe0813ee133adfcc9L598

By removing the else, a compaction may be triggered after writing an L0 file. This will result in a compaction if more than 4 L0 files are present. Before, more and more L0 files would be written but the database would never check if there is a need to compact the L0 files.

Ah! So now needs_compaction will be run even though self.imm.is_some() evaluated to true. Got it.

dermesser commented 1 year ago

Ah! So now needs_compaction will be run even though self.imm.is_some() evaluated to true. Got it.

In fact, once compact_memtable() has returned, self.imm.is_some() is false, so it makes sense to re-check.

Sword-Smith commented 1 year ago

Running a version with the do-more-compactions version now. Will report back what I see.

The code I'm running to test it can be found here: https://github.com/Neptune-Crypto/neptune-core/tree/thv/fix-walletdb-disk-usage-issue

Sword-Smith commented 1 year ago

Looks promising: After running the do-more-compactions version on two machines for six hours, I haven't been above a 3x of the compacted size. I can run it for a while longer though.

dermesser commented 1 year ago

Thank you for testing it. I will merge it and soon release it together with the new Compressor API (#33) as a new major version, so it definitely won't break anyone who cares.