Closed ianadavies closed 4 years ago
Tested this. It seemed to be slower with manual profiling (stopwatching, average of 5 queries don't judge lol).
My GetHistory call with 10k entries and ~215 sends went from ~2.8s to ~3.5s.
I looked into this and tried to optimise... Avoid the ToLists and multiple settting on the dictionary, removed the lookup entirely and made the cache a normal Dictionary
, we don't need concurrent.
New code would look like this:
private static readonly Dictionary<Script, string> ScriptPubKeyCache = new Dictionary<Script, string>();
if (transactionData.SpendingDetails == null || this.ScriptAddressReader == null)
return res;
foreach (var paymentDetails in res)
{
if (ScriptPubKeyCache.TryGetValue(paymentDetails.DestinationScriptPubKey, out string storedAddress))
{
paymentDetails.DestinationAddress = storedAddress;
}
else
{
string address = this.ScriptAddressReader.GetAddressFromScriptPubKey(this.Network, paymentDetails.DestinationScriptPubKey);
ScriptPubKeyCache[paymentDetails.DestinationScriptPubKey] = address;
paymentDetails.DestinationAddress = address;
}
}
return res;
Using the above code, the GetHistory call was marginally faster. Seemed to be ~2.6s.
But I don't think those results are significant enough to warrant putting in at this stage.
I concur with @quantumagi, if there is a worthwhile performance benefit from introducing this cache, then I would suggest utilising an LRU cache implementation with a bounded maximum size. See the MemorySizeCache
/ MemoryCountCache
in Stratis.Bitcoin.Utilities
as possible ready-made implementations.
3.0.6.0 is closed, so I think we can close this. If you're still keen to get this in @ianadavies feel free to open a PR to 3.0.7.0
Performance Improvement. - This PR caches the address read from the
ScriptPubKey
when using theScriptAddressReader
. This is currently done for everyHDPayment
Record and is an expensive operation.