Remove DB flush in read-only mode

Namecoin, NMControl
Post Reply
domob
Posts: 1129
Joined: Mon Jun 24, 2013 11:27 am
Contact:

Remove DB flush in read-only mode

Post by domob »

Take a look at this: https://github.com/namecoin/namecoin/pull/73

Currently, even databases that are in read-only mode call BDB's txn_checkpoint when they are closed / run out of scope. While txn_checkpoint presumably does nothing when nothing has changed, it still seems to consume at least some resources and perform some disk accesses in any case. This patch disables the call entirely when the database is in read-only mode.

I stumbled upon this while working on improving the nameindex rescan performance in Huntercoin, where profiling showed that this was one (but not the only) bottleneck. Other improvements specifically for nameindex rescan will be ported over, too, but for now I suggest to test and include this patch as it has the potential to increase performance overall (although I have no measurable results to present so far).

Even though it should be "clear" that this patch is ok (since obviously no txn_checkpoint / "flushing" should be needed in read-only mode), please make sure to test it a bit to ensure that it really doesn't cause any problems. (I've been running it for half a day already on both Huntercoin and Namecoin without noticing any bad things happening.)
BTC: 1domobKsPZ5cWk2kXssD8p8ES1qffGUCm | NMC: NCdomobcmcmVdxC5yxMitojQ4tvAtv99pY
BM-GtQnWM3vcdorfqpKXsmfHQ4rVYPG5pKS
Use your Namecoin identity as OpenID: https://nameid.org/

domob
Posts: 1129
Joined: Mon Jun 24, 2013 11:27 am
Contact:

Re: Remove DB flush in read-only mode

Post by domob »

Some remarks to the history of the code in question: Currently, the code sets the "flush interval" by default to two minutes and changes this to one minute in read-only mode. This is very curious by itself - and was introduced by phelix' commit https://github.com/namecoin/namecoin/co ... 8a8264199f.

Previously, the "default" flush interval was "immediately", so that setting it to one minute in read-only mode was actually really a performance-improvement in read-only mode. The check for read-only mode itself was introduced by Satoshi himself in https://github.com/namecoin/namecoin/co ... 2fd012e7f7. Incidentally, this is also the same commit where he added the (never used) "getheaders" network command and CBlockIndex::GetBlockHeaders -- twice dead code (it seems that he intended this to be extended in a full feature later but never came around to it) that is removed / disabled in my blkindex.dat patch.
BTC: 1domobKsPZ5cWk2kXssD8p8ES1qffGUCm | NMC: NCdomobcmcmVdxC5yxMitojQ4tvAtv99pY
BM-GtQnWM3vcdorfqpKXsmfHQ4rVYPG5pKS
Use your Namecoin identity as OpenID: https://nameid.org/

snailbrain
Posts: 309
Joined: Tue Jul 19, 2011 9:33 pm

Re: Remove DB flush in read-only mode

Post by snailbrain »

brill work domob

phelix
Posts: 1634
Joined: Thu Aug 18, 2011 6:59 am

Re: Remove DB flush in read-only mode

Post by phelix »

domob wrote:Some remarks to the history of the code in question: Currently, the code sets the "flush interval" by default to two minutes and changes this to one minute in read-only mode. This is very curious by itself - and was introduced by phelix' commit https://github.com/namecoin/namecoin/co ... 8a8264199f.

Previously, the "default" flush interval was "immediately", so that setting it to one minute in read-only mode was actually really a performance-improvement in read-only mode. The check for read-only mode itself was introduced by Satoshi himself in https://github.com/namecoin/namecoin/co ... 2fd012e7f7. Incidentally, this is also the same commit where he added the (never used) "getheaders" network command and CBlockIndex::GetBlockHeaders -- twice dead code (it seems that he intended this to be extended in a full feature later but never came around to it) that is removed / disabled in my blkindex.dat patch.
You make it sound like my changes to this function made things worse but I still think they were an improvement. I was probably unsure about whether it was and advantage to flush readonly mode files early so I left it alone. Note that this particular change was only to clean up the code but there should be an earlier one that prevented at least one file only present in Namecoin but not in Bitcoin from being flushed all the time.

edit: https://github.com/namecoin/namecoin/co ... 3f7b5b7ee5
Certainly can not hurt if you take a look at it, too :mrgreen:
nx.bit - some namecoin stats
nf.bit - shortcut to this forum

domob
Posts: 1129
Joined: Mon Jun 24, 2013 11:27 am
Contact:

Re: Remove DB flush in read-only mode

Post by domob »

phelix wrote:
domob wrote:Some remarks to the history of the code in question: Currently, the code sets the "flush interval" by default to two minutes and changes this to one minute in read-only mode. This is very curious by itself - and was introduced by phelix' commit https://github.com/namecoin/namecoin/co ... 8a8264199f.

Previously, the "default" flush interval was "immediately", so that setting it to one minute in read-only mode was actually really a performance-improvement in read-only mode. The check for read-only mode itself was introduced by Satoshi himself in https://github.com/namecoin/namecoin/co ... 2fd012e7f7. Incidentally, this is also the same commit where he added the (never used) "getheaders" network command and CBlockIndex::GetBlockHeaders -- twice dead code (it seems that he intended this to be extended in a full feature later but never came around to it) that is removed / disabled in my blkindex.dat patch.
You make it sound like my changes to this function made things worse but I still think they were an improvement. I was probably unsure about whether it was and advantage to flush readonly mode files early so I left it alone. Note that this particular change was only to clean up the code but there should be an earlier one that prevented at least one file only present in Namecoin but not in Bitcoin from being flushed all the time.
Yes, sorry that it sounded that way - it didn't make things worse and I think your change was a very good one. It just introduced a somewhat odd situation, where read-only files were flushed more often than read-write files. But it wasn't made worse by your patch, your patch just improved read-write files while keeping read-only the same. ;)
BTC: 1domobKsPZ5cWk2kXssD8p8ES1qffGUCm | NMC: NCdomobcmcmVdxC5yxMitojQ4tvAtv99pY
BM-GtQnWM3vcdorfqpKXsmfHQ4rVYPG5pKS
Use your Namecoin identity as OpenID: https://nameid.org/

phelix
Posts: 1634
Joined: Thu Aug 18, 2011 6:59 am

Re: Remove DB flush in read-only mode

Post by phelix »

Actually I wonder what flushing a read only file does. Clear the cache? Do we want that?
nx.bit - some namecoin stats
nf.bit - shortcut to this forum

domob
Posts: 1129
Joined: Mon Jun 24, 2013 11:27 am
Contact:

Re: Remove DB flush in read-only mode

Post by domob »

phelix wrote:Actually I wonder what flushing a read only file does. Clear the cache? Do we want that?
The "read-only" thing is only a flag within Namecoin and not reflected in the underlying BDB handle. The BDB handle is only opened once and reused for multiple CDB instances in Namecoin - and when any one of these is "closed", the code in question does a txn_checkpoint. (Which flushes the logs (at least partially - I don't really know the details), but even without it, the database "should" still be consistent when the application is killed.)

So when we flush also for read-only databases, it just checks more frequently whether or not to do the actual flush (depending on the time of the last one) - and presumably this check alone can be quite performance critical if done very often (for instance, whenever we have "CTxDb txdb('r')" in the code while verifying transactions).

This change may actually lead to less real flushes (not only avoiding superfluous checks), but it is still guaranteed that whenever something was changed (i. e., a read-write DB used), then the DB is flushed when the last flush is more than the specified number of minutes ago. (In particular, wallet.dat still flushes after every read-write instance is closed immediately.)
BTC: 1domobKsPZ5cWk2kXssD8p8ES1qffGUCm | NMC: NCdomobcmcmVdxC5yxMitojQ4tvAtv99pY
BM-GtQnWM3vcdorfqpKXsmfHQ4rVYPG5pKS
Use your Namecoin identity as OpenID: https://nameid.org/

Post Reply