Hold LOCKs only as long as necessary

[dexX7]

As outlined in #84,gettransactions_MP and other parts currently lock certain sections longer than necessary.

In case of gettransactions_MP an additional improvement could be to restructure the command, such that we lock the wallet, collect the wallet transactions, release the lock, and then process the transactions, instead of holding the lock for the whole time.

I'm going to tackle it, once the RPC branch is in.

[zathras-crypto]

currently lock certain sections longer than necessary.
additional improvement could be to restructure the command, such that we lock the wallet, collect the wallet transactions, release the lock, and then process the transactions, instead of holding the lock for the whole time.

I'm not sure if this is possible mate - how would you go about it?

Let's say we do a listtransactions_MP and we're looking for the most recent 10 Omni transactions - we don't know if we'll have to look at the last 10, the last 100, or the last 1000 wallet transactions to find them.

I'm of course open to any ideas you have here mate, but I didn't think we could optimize any more until we changed architecture for data storage.

[dexX7]

Very good point about not knowing how wallet transactions are really relevant.

Let's look at it this way: we do a lot of things, like querying the database, parsing transactions, converting some data into JSON objects, and all this has basically nothing to do with the wallet, so the lock is "unused" (or unjustified) during that time.

I haven't really thought about specifics yet. A probably straight forward approach could be to lock the wallet, fetch the wallet transactions, and release the lock. If, for some reason, this isn't safe, because the wallet may interfere with the pointers to these wallet transactions, we could create copies of them before going on.

[zathras-crypto]

That's the issue - you don't want to be creating say 10,000 copies of wallet transactions (you never know if you'll need them)... I have another approach in mind, bear with me for a few hours and I'll push up an update to walletcache.cpp which will provide a cached list of transactions that affect the wallet, which can be used to optimize these other calls :)

[dexX7]

Haha, this is slowly getting out of hands: we have a transaction history cache, one for trades, probably soon another one for the overview transactions (they could share the other cache actually), I have a coins view cache pending, and you one for wallet balances. Oh well.. :)

Jokes aside: all this provided very, very significant speed boosts, so it's a good thing. :)

However, I suggest to hold back (non trivial) structural updates of the RPC stuff until your RPC branch is in.

[zathras-crypto]

Indeed hehe - FYI the idea is to provide a data source via walletcache.cpp that is a drop in replacement for iteration - instead of all wallet transactions we iterate only Omni wallet transactions.

Let me see what I come up with, I'm on a roll here I think hehe ;)

[dexX7]

Might be easier to cache CMPTransactions then?

[zathras-crypto]

That's probably a little more in depth than I was going for, but we could do I guess?

FYI I was simply going to add a vector of uint256 that gets updated via the transaction handler when processed if it was in the wallet.

What do you propose? Perhaps a map of uint256 & CMPTransaction that cached each transaction object?

[dexX7]

FYI I was simply going to add a vector of uint256 that gets updated via the transaction handler

Ah, this sounds pretty straight forward approach.

... when processed if it was in the wallet.

Keep in mind: there are also STO transactions.

Maybe we should get something like bool InvolvesMe(theTransaction) or so?

What do you propose? Perhaps a map of uint256 & CMPTransaction that cached each transaction object?

I'm not really sure, and this was just an idea, but I initially assumed you wanted to cache CWalletTx instead of hashes.

[zathras-crypto]

I'm not really sure, and this was just an idea, but I initially assumed you wanted to cache CWalletTx instead of hashes.

Ahh, for the initial I figured we'd get away with just the hashes. If I exaggerate a scenario hopefully it'll help explain:

1. I send 10 Omni transactions
2. I send 10,000 Bitcoin transactions
3. I call listtransactions_MP with the default params (returns 10 most recent txs)

The code as present will need to loop through all 10K transactions from 2) and still hasn't found any recent Omni transactions. After going through those 10K transactions it'll then find those 10 Omni transactions I sent first. It'll do this every time I make the listtransactions_MP call.

A list of hashes is sufficient to avoid that I believe :)

[dexX7]

Sounds good! :)

[dexX7]

Speaking of locks.. currently all Omni RPC commands are tagged as non-threadsafe, and as result, every call auto-locks cs_main and cs_wallet.

We should ultimately lock on a case-by-case basis, and only for as long as necessary, so multiple RPC threads have an effect.

[dexX7]

Oh, look at this, when fetching simple send wallet transactions on mainnet, then around 99 % of the time is spent to populate the transaction object, and:

populateRPCTransactionObject():

  - GetTransaction():         0.191 ms
  - p_txlistdb->exists():     0.025 ms
  - ParseTransaction():       0.884 ms
  - getValidMPTX():           0.089 ms
  - isCrowdsalePurchase():   89.296 ms  <---
  - JSON conversion:          0.049 ms
  - anything inbetween:       0.096 ms

Total: 90.63 ms

[zathras-crypto]

Yeah that call is horribly inefficient (see the comment hehe) - there is a large amount of work that is done to essentially "find" any crowdsales that the simple send could have been involved in because we don't currently store a list of atomic actions per message. This would be one of those things that comes in with the discussion on restructuring the stored data, where we have direct access to xyz rather than reconstructing it from the blockchain.

[zathras-crypto]

Note, I can cache this too (using a map holding a matching between simple send txids & property ids their respective crowdsale purchase) which drastically would cut down on the amount of iterating SP databases and make the call much faster.

[zathras-crypto]

I pushed up a couple of commits by the way, one with walletcache.cpp extended to cover a TXID cache and a second to demo it via the listtransactions_MP call.

[dexX7]

Yeah that call is horribly inefficient (see the comment hehe)

I digged deeper, and it's pretty surprising, but not unexpected:

Most of the time goes into 205 getSP(), but only one call takes about 90 % of all: MaidSafeCoin.

To drill it further down, and locate the actual source:

• 16 ms for json_spirit::read_string
• 57 ms for CMPSPInfo::Entry::fromJSON()