mimblewimble / grin

Minimal implementation of the Mimblewimble protocol.
https://grin.mw/
Apache License 2.0
5.04k stars 990 forks source link

Profile retrieving UTXOs from the data store #237

Closed ignopeverell closed 6 years ago

ignopeverell commented 6 years ago

Running grin wallet info can take a very long time when there are hundreds or thousands of UTXOs. Normally reading those from rocksdb (even including related headers) should be extremely fast. Profile it and check what's wrong.

yeastplume commented 6 years ago

Well that's interesting, from an initial perf run it looks like the bulk of time is spent parsing a hex string

ignopeverell commented 6 years ago

I was half expecting something silly like that. Those simple bottlenecks are the best, everything after that gets faster!

yeastplume commented 6 years ago

I need to do another run or so with a much larger UTXO set, had one preparing for a few hours.. I'll run another perf against it using a swapped-out version of from_hex and see whether it makes a difference.

yeastplume commented 6 years ago

whoops... nope. I replaced the from_hex function and it was only a minor speed increase. I did another with about 234 coinbase transactions built (nothing else.. ) a lot of slowdown on the wallet side as well churning something before even calling the UTXO api.

This is the graph from get_utxo:

yeastplume commented 6 years ago

So it looks as if the Readable trait implementation for HashSum is where the most slowdown is on the server side. Will post the graph from the wallet side shortly.

ignopeverell commented 6 years ago

Mmmh and looks like most of it is spent in secp256k1? Only thing that HashSum::read does is build a Commitment, doesn't seem that it should trigger that much processing.

yeastplume commented 6 years ago

Indeed... here's another one from the wallet.. this is from the moment you start the wallet info command until the first hit against the api server, again, most of the time in secp256k1. This was a good 10-15 seconds

wallet-refresh-outputs

yeastplume commented 6 years ago

Okay, this one in particular doesn't look like an issue in with libsecp speed, it's just being called a lot. Looks like an implementation for finding derived keys that needs speeding up.

https://github.com/mimblewimble/grin/blob/8f0373b81e1e882240eec95161c054ce0f571607/keychain/src/keychain.rs#L96

yeastplume commented 6 years ago

Okay, the wallet side was a fairly easy fix... server still seems sluggish, and it appears get_unspent is where the major inefficiency is.

yeastplume commented 6 years ago

Got it. Have a more accurate perf report here:

secp_issue

https://github.com/mimblewimble/grin/blob/8f0373b81e1e882240eec95161c054ce0f571607/core/src/core/transaction.rs#L565

It's the fact that secp256k1 is being initialized each and every time it's being called. There should probably be a common static instance stored somewhere.

Nearing the end of my useful coding period for today, if you want to look at it @ignopeverell

ignopeverell commented 6 years ago

Sorry I think I'm a little under the weather. I've been staring at this for the last 30 min and fighting the borrow checker with little result. I might take it easy for a couple days and get back to it when I'm less foggy. Thanks a lot for profiling all this and tracking it down! I'll get a fix ASAP.

yeastplume commented 6 years ago

That's no problem, I've added a solution to the PR I have outstanding which addresses that issue. https://github.com/mimblewimble/grin/pull/238

That PR greatly speeds things up overall, but now I can see most time is now spent in json serializing, so I'm sure there's more room for optimization.

yeastplume commented 6 years ago

JSON serialising time is down to serialising/deserialising rangeproofs as part of every output, even when it's not necessary. I think I'll leave it at the last commit for now, greatly speeded up but still room for improvement.. in particular we need to avoid passing rangeproofs around when they're not necessary, and should probably have a way to deserialise Outputs without necessarily reading the entire range proof in all cases.

yeastplume commented 6 years ago

Think this is all to close now, if everyone is seeing vastly quicker results from wallet info?

ignopeverell commented 6 years ago

Night and day. Thanks for both profiling and fixing!