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

Add Binary64 option for account data #11474

Merged
merged 3 commits into from
Aug 9, 2020

Conversation

sakridge
Copy link
Member

@sakridge sakridge commented Aug 8, 2020

Problem

bs58 encoding of large data is slow

Summary of Changes

Add base64 option and limit bs58 requests to account data less that 128 bytes.

Fixes #10140

core/src/rpc.rs Outdated Show resolved Hide resolved
@mvines mvines requested a review from CriesofCarrots August 8, 2020 16:57
@mvines
Copy link
Member

mvines commented Aug 8, 2020

We'll need an update to docs/src/apps/jsonrpc-api.md here eventually. Also let's convert solana account/solana vote-account/solana stake-account over to use base64 (there may be a couple other commands in the cli that will now die with the 128 byte limit too)

@mvines
Copy link
Member

mvines commented Aug 8, 2020

Allowing the user to optionally supply an "offset"/"length" seems pretty nice too, for when they know they only need a slice of a large account

@sakridge sakridge force-pushed the add-hex-encode branch 4 times, most recently from ae10f23 to 7d79eb8 Compare August 8, 2020 17:54
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Approach looks fine to me. I haven't examined to see why the cli/deploy test is unhappy yet.
I like the idea of an offset/length specification, and could help with that in a follow-up

docs/src/apps/jsonrpc-api.md Outdated Show resolved Hide resolved
@sakridge
Copy link
Member Author

sakridge commented Aug 8, 2020

Approach looks fine to me. I haven't examined to see why the cli/deploy test is unhappy yet.
I like the idea of an offset/length specification, and could help with that in a follow-up

rpc_client was trying to decode as Binary instead of Binary64. After I fixed that it seems to be ok, although my fix is a bit hacky.
773f002

@CriesofCarrots
Copy link
Contributor

rpc_client was trying to decode as Binary instead of Binary64. After I fixed that it seems to be ok, although my fix is a bit hacky.

Ah, serde_json must go with the first enum variant that matches type 😕
I'm fine with your fix; the less-hacky ways I've thought to do this are a lot less concise.

Looks like you just need to update this line to Binary64 to fix the CI failure:

let rpc_nonce_account = UiAccount::encode(nonce_account, UiAccountEncoding::Binary, None);

@mvines
Copy link
Member

mvines commented Aug 9, 2020

I like the idea of an offset/length specification, and could help with that in a follow-up

@CriesofCarrots - sure, can you queue this up for next week. We now do have a real use case where it would be more efficient for the client (and the node) if they were able to only request partial account data

docs/src/apps/jsonrpc-api.md Outdated Show resolved Hide resolved
core/src/rpc.rs Outdated Show resolved Hide resolved
@sakridge
Copy link
Member Author

sakridge commented Aug 9, 2020

test bench_encode_base58 ... bench:  20,546,592 ns/iter (+/- 65,220)
test bench_encode_base64 ... bench:   6,253,972 ns/iter (+/- 107,235)

base64 does seem quite a bit faster.

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #11474 into master will decrease coverage by 0.0%.
The diff coverage is 78.5%.

@@            Coverage Diff            @@
##           master   #11474     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         323      323             
  Lines       74965    74982     +17     
=========================================
- Hits        61325    61307     -18     
- Misses      13640    13675     +35     

@mvines
Copy link
Member

mvines commented Aug 9, 2020

Definitely faster. I did a ./run.sh with/without this PR and timed solana accounts TokenSVp5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o -- 8 seconds for base58, 3.5 seconds for base64.

I wonder if we could do even better with just hex encoding (as an alternative for when increase in encoding size doesn't matter, like small partial account reads) , would that be quick to bench @sakridge?

@sakridge
Copy link
Member Author

sakridge commented Aug 9, 2020

Definitely faster. I did a ./run.sh with/without this PR and timed solana accounts TokenSVp5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o -- 8 seconds for base58, 3.5 seconds for base64.

I wonder if we could do even better with just hex encoding (as an alternative for when increase in encoding size doesn't matter, like small partial account reads) , would that be quick to bench @sakridge?

Hex seems worse:

test bench_encode_base58 ... bench:  73,587,849 ns/iter (+/- 531,971)
test bench_encode_base64 ... bench:       3,518 ns/iter (+/- 31)
test bench_encode_hex    ... bench:      26,636 ns/iter (+/- 84)

That's 8k of random data which with base58 is all I can stand to wait for. It really falls apart with random data.

@mvines
Copy link
Member

mvines commented Aug 9, 2020

Hex is worse 🤯.

@sakridge
Copy link
Member Author

sakridge commented Aug 9, 2020

Hex is worse 🤯.

yep @mvines any blockers for merging this?

Copy link
Member

@mvines mvines left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@sakridge sakridge merged commit 068d23f into solana-labs:master Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC uses base58 for large binary leading to slow response
3 participants