Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add a lookup cache for txOutputs #4951

Closed

Conversation

chimp1984
Copy link
Contributor

TxOutputs are requested very often and the previous iterations
accumulate lot of cpu time as profiler shows.

TxOutputs are requested very often and the previous iterations
accumulate lot of cpu time as profiler shows.
@chimp1984
Copy link
Contributor Author

Here are performance test results.

PR version:
100 calls of daoStateService.getTxOutputsByTxOutputType(TxOutputType.BSQ_OUTPUT) took 2508 ms
100000 calls of daoStateService.existsTxOutput(TxOutputKey) took 2 ms
100000 calls of daoStateService.getTxOutput(TxOutputKey) took 3 ms

Master:
100 calls of daoStateService.getTxOutputsByTxOutputType(TxOutputType.BSQ_OUTPUT) took 4206 ms
100 calls of daoStateService.existsTxOutput(TxOutputKey) took 2907 ms
100 calls of daoStateService.existsTxOutput(TxOutputKey) took 2946 ms

The daoStateService.getTxOutputsByTxOutputType call takes about 30 ms which is very expensive and
requires more improvement but as it iterates the cache map it is expected to has little impact.

The other 2 methods havea huge difference.
Note that iterations have been increased from 100 to 100000 to get some meaningful time log.

I think we should measure all methods and find more hotspots...

As its a risky domain, I leave it to maintainers to merge the PR or delay it for later.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Dec 15, 2020

I checked how often the methods are called:
getTxOutputsByTxOutputType is called about 500 times after a few minutes and clicking
though most DAO screens resulting in 15 sec accululated CPU time.

getTxOutput and existsTxOutput is only used from the BsqCoinSelector and NonBsqCoinSelector
in edge cases, so only relevant for users who have BSQ in their wallet. I got existsTxOutput called
3 times and getTxOutput called 0 times with a live app with BSQ.

So unfortunately the big improvements will not have much impact as not called often
and the one which is called more often brings not so much difference.

So not sure if the added risk and the added memory footprint by adding a cache as
additional data structure (150k objects) are worth it.

I would tend to postpone that improvement to find the more severe bottlenecks. I am still a bit surprised as profiler
reported the getTxOutputsByTxOutputType method as hotspot.

@chimp1984 chimp1984 changed the title Add a lookup cache for txOutputs [WIP] Add a lookup cache for txOutputs Dec 15, 2020
@chimp1984 chimp1984 marked this pull request as draft December 16, 2020 15:58
@stale
Copy link

stale bot commented Jan 17, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Jan 17, 2021
@chimp1984 chimp1984 closed this Jan 18, 2021
@chimp1984 chimp1984 deleted the add-txOuputCache-to-daoState branch November 18, 2021 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant