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.

Sign up for GitHub

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

Jump to bottom

[WIP] status-im/status-react#9203 BalanceAt caching #1744

Closed
wants to merge 1 commit into from
Closed

[WIP] status-im/status-react#9203 BalanceAt caching #1744

wants to merge 1 commit into from

Conversation

rasom
Copy link
Collaborator

@rasom rasom commented Dec 18, 2019 •
edited by 0xc1c4da
Loading

status-im/status-mobile#9203

what's already fixed here:

  • we do not request history twice for the same address as we did
  • we do not request balance for a specific block more then once, depending on circumstances we could do this up 23-24 times for a specific block, but in average it reduces the number of eth_getBalance requests in 3-4 times

status: ready

@status-github-bot
Copy link

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?

@status-im-auto
Copy link
Member

status-im-auto commented Dec 18, 2019 •
edited
Loading

Jenkins Builds

Click to see older builds (24)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 05ac41a #1 2019-12-18 11:07:41 ~1 min linux 📦zip
✔️ 05ac41a #1 2019-12-18 11:12:03 ~5 min ios 📦zip
✔️ 05ac41a #1 2019-12-18 11:15:48 ~9 min android 📦aar
✔️ 43e8d7b #2 2019-12-18 11:13:53 ~43 sec linux 📦zip
✔️ 43e8d7b #2 2019-12-18 11:16:30 ~3 min ios 📦zip
✔️ 43e8d7b #2 2019-12-18 11:23:14 ~7 min android 📦aar
✔️ 49171d1 #3 2019-12-18 15:49:09 ~41 sec linux 📦zip
✔️ 49171d1 #3 2019-12-18 15:53:43 ~5 min ios 📦zip
✔️ 49171d1 #3 2019-12-18 15:55:54 ~7 min android 📦aar
✔️ c5fd527 #4 2019-12-18 15:50:54 ~30 sec linux 📦zip
✔️ c5fd527 #4 2019-12-18 15:56:21 ~2 min ios 📦zip
✔️ c5fd527 #4 2019-12-18 16:03:49 ~7 min android 📦aar
✔️ f3171fd #5 2019-12-18 16:01:58 ~48 sec linux 📦zip
✔️ f3171fd #5 2019-12-18 16:03:56 ~2 min ios 📦zip
✔️ f3171fd #5 2019-12-18 16:11:43 ~7 min android 📦aar
✔️ 1b066d5 #6 2019-12-19 10:32:31 ~1 min linux 📦zip
✔️ 1b066d5 #6 2019-12-19 10:34:09 ~2 min ios 📦zip
✔️ 1b066d5 #6 2019-12-19 10:39:44 ~8 min android 📦aar
✔️ ffa7903 #7 2019-12-20 17:52:55 ~38 sec linux 📦zip
✔️ ffa7903 #7 2019-12-20 17:54:53 ~2 min ios 📦zip
✔️ ffa7903 #7 2019-12-20 17:59:17 ~7 min android 📦aar
✔️ dea56e0 #8 2019-12-20 18:05:57 ~1 min linux 📦zip
✔️ dea56e0 #8 2019-12-20 18:07:25 ~2 min ios 📦zip
✔️ dea56e0 #8 2019-12-20 18:12:41 ~7 min android 📦aar
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ a1c6bb4 #9 2019-12-20 18:10:35 ~36 sec linux 📦zip
✔️ a1c6bb4 #9 2019-12-20 18:12:55 ~2 min ios 📦zip
✔️ a1c6bb4 #9 2019-12-20 18:20:01 ~7 min android 📦aar
✔️ 9f60462 #10 2020-01-02 09:37:53 ~41 sec linux 📦zip
✔️ 9f60462 #10 2020-01-02 09:43:10 ~5 min ios 📦zip
✔️ 9f60462 #10 2020-01-02 09:44:57 ~7 min android 📦aar

@rasom rasom self-assigned this Dec 18, 2019
adambabik
adambabik reviewed Dec 18, 2019
View reviewed changes
Copy link
Contributor

@adambabik adambabik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I would try to put the cache into a struct and assign an owner of it.

api/geth_backend.go Outdated Show resolved Hide resolved
api/geth_backend.go Outdated Show resolved Hide resolved
api/geth_backend.go Outdated Show resolved Hide resolved
services/wallet/concurrent.go Outdated Show resolved Hide resolved
services/wallet/concurrent.go Outdated
balanaceMapLock.RLock()
defer balanaceMapLock.RUnlock()

balance, exists := balancesCache[account][blockNumber]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
balance, exists := balancesCache[account][blockNumber]
return balancesCache[account][blockNumber]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I do so how do I check whether map contains the value? because I also have to change signature of the function and it wouldn't return that ok value or something

services/wallet/concurrent.go Outdated
defer balanaceMapLock.RUnlock()

balance, exists := balancesCache[account][blockNumber]
return balance, exists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return balance, exists

services/wallet/concurrent.go Outdated Show resolved Hide resolved
services/wallet/concurrent.go Outdated Show resolved Hide resolved
@adambabik adambabik changed the title status-im/status-react#9203 BalanceAt caching [WIP] status-im/status-react#9203 BalanceAt caching Dec 18, 2019
@adambabik
Copy link
Contributor

Sorry, I didn't notice it's WIP...

@rasom
Copy link
Collaborator Author

rasom commented Dec 18, 2019

Generally, I would try to put the cache into a struct and assign an owner of it.

@adambabik not like I really understand what you mean :D I could add the last thing I wanted to have in this PR (checking of eta_getTransactionCount in case if balance was zero in both from and to blocks of given blocks range we check for transactions) and then you could rewrite it the way it pleases golang dev eye, because for me it's kind of impossible task at the moment

@rasom rasom force-pushed the get-balance-caching branch 2 times, most recently from 49171d1 to c5fd527 Compare December 18, 2019 15:50
@rasom rasom mentioned this pull request Dec 18, 2019
Closed
@rasom
Copy link
Collaborator Author

rasom commented Dec 19, 2019

@adambabik
Hey Adam, could you take a look again? I hope it looks a bit better now, let me know what has to be changed.

@rasom rasom requested a review from adambabik December 19, 2019 10:35
adambabik
adambabik reviewed Dec 20, 2019
View reviewed changes
Copy link
Contributor

@adambabik adambabik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👏 A few details that can be improved.

services/wallet/balance_cache.go Show resolved Hide resolved
services/wallet/balance_cache.go Outdated Show resolved Hide resolved
services/wallet/balance_cache.go Outdated Show resolved Hide resolved
services/wallet/balance_cache.go Outdated Show resolved Hide resolved
services/wallet/balance_cache.go Outdated Show resolved Hide resolved
services/wallet/balance_cache.go Outdated Show resolved Hide resolved
services/wallet/commands.go Outdated Show resolved Hide resolved
@rasom
Copy link
Collaborator Author

rasom commented Dec 20, 2019

@adambabik should be fixed now

@weekly-digest weekly-digest bot mentioned this pull request Dec 22, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Jan 5, 2020
Closed
@jakubgs jakubgs force-pushed the develop branch 4 times, most recently from a726462 to 330d177 Compare January 11, 2020 19:24
@weekly-digest weekly-digest bot mentioned this pull request Jan 12, 2020
Closed
@rasom rasom closed this Jan 15, 2020
@rasom rasom deleted the get-balance-caching branch January 15, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambabik adambabik adambabik left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rasom @status-im-auto @adambabik