WIP: Different optimisations #16
Conversation
| @@ -449,7 +442,9 @@ public TransactionConfidence duplicate() { | |||
| * explicitly, more precise control is available. Note that this will run the listeners on the user code thread. | |||
| */ | |||
| public void queueListeners(final Listener.ChangeReason reason) { | |||
| for (final ListenerRegistration<Listener> registration : listeners) { | |||
| //Replaced with for loop as iterator init consumpted 11% of cpu time | |||
There was a problem hiding this comment.
This is very unbelievable to me. I suspect your measurement to be wrong as iterators on lists are basically as fast as it can get. I did some very basic simulation with the actual classes involved. and with a list of 10k, measurement is single digit. With 1M, setup takes forever. With 300k, setup takes 21s and iterations both less than 10ms.
Now this might be a special issue with Android or something but I would like to rule out false measurement before adding such a degradation. After all, it's even by your measure only 12% compared to some cases where we see 97% of potential.
There was a problem hiding this comment.
The problem is that assumption that there's a lot of listeners is totally wrong. Number of listeners never exceeds two, so we would never loose in using basic loop, while as this method is called very frequently we would loose on object construction.
But would test tomorrow again and place a profiling here.
There was a problem hiding this comment.
I did more tests, trying to provoke many runs of a short list but I see less than a 10% difference there. If so much time is spent in this part, maybe we can improve it with some kind of batching, as apparently the listeners mostly ignore what they get.
There was a problem hiding this comment.
Ok, unable to reproduce now. Let's cancel this.
| else if (isPayToScriptHash()) | ||
| return chunks.get(1).data; | ||
| else | ||
| return null; |
There was a problem hiding this comment.
Instead of creating a new method, we could speed up the Exception-throwing in the other method using static Exceptions without stacktrace.
| lock.lock(); | ||
| try { | ||
| return hashToKeys.get(ByteString.copyFrom(pubkeyHash)); | ||
| return hashToKeys.get(keyHashString); |
There was a problem hiding this comment.
I think it is safe to remove ByteString from the whole class. The class never exposes the hashmap keys and when putting them into the hashmap, it can create a copy of the byte[]s for immutability.
| @@ -1658,11 +1658,14 @@ boolean isTxConsistent(final Transaction tx, final boolean isSpent) { | |||
| boolean isActuallySpent = true; | |||
| for (TransactionOutput o : tx.getOutputs()) { | |||
| if (o.isAvailableForSpending()) { | |||
There was a problem hiding this comment.
the following change is not intuitively correct. Is o.getSpentBy() != null such a common case that it matters enough to change it?
There was a problem hiding this comment.
I suppose that the order of two if checks changes nothing?
But I've decided to follow method docu and stop method execution if even a one output is available. So you're right could just write return true at the end of this if branch without changing anything else, would do.
| } | ||
| } | ||
|
|
||
| informConfidenceListenersIfNotReorganizing(); | ||
| maybeQueueOnWalletChanged(); | ||
|
|
||
| if (hardSaveOnNextBlock) { | ||
| saveNow(); | ||
| //saveNow(); |
There was a problem hiding this comment.
hardSaveOnNextBlock gets completely overridden here. at least the if is pointless.
There was a problem hiding this comment.
That was not branch for PR... Some more work supposed to be :)
I suppose we should discuss about this saving...
| @@ -2884,6 +2879,9 @@ public void run() { | |||
| public Set<Transaction> getTransactions(boolean includeDead) { | |||
| lock.lock(); | |||
| try { | |||
| if (includeDead) { | |||
There was a problem hiding this comment.
includeDead can be removed further down this method. Else, probably a valid improvement.
| @@ -753,7 +757,6 @@ private void readConfidence(final NetworkParameters params, final Transaction tx | |||
| log.warn("Have depth but not BUILDING for tx {}", tx.getHashAsString()); | |||
| return; | |||
| } | |||
| confidence.setDepthInBlocks(confidenceProto.getDepth()); | |||
There was a problem hiding this comment.
Changing the protobuf? Is this what is causing the many newly broken tests?
There was a problem hiding this comment.
Damn "newly broken tests". Tests fail very randomly. I don't know yet under which conditions I only get 12 failing tests but so far I only and often observed it on our multi-hd-accounts branch.
There was a problem hiding this comment.
I suppose that using context which is thread specific for passing number of blocks is not a good idea...
Still thinking how to fix this.
There was a problem hiding this comment.
Just tested multiple_hd_accounts, received 38 tests failed...
Another start 28...
Goal of this PR is to improve the performance, especially for initial sync of huge wallets.
Things being evaluated are: