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

feat: upload multiple asset chunks in the same call #3947

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

sesi200
Copy link
Contributor

@sesi200 sesi200 commented Oct 9, 2024

Description

The frontend canister sync now tries to batch multiple small content chunks into a single call using the create_chunks method added in #3898.
This should lead to significantly faster upload times for frontends with many small files.

Also had to bump the api_version of the asset canister to build in backwards compatibility for the asset sync

Fixes SDK-1769

How Has This Been Tested?

Covered by existing e2e.
Manually tested repeatedly with many different average file sizes. Command to test:

for i in $(seq 1 50); do dd if=/dev/urandom of="src/hello_frontend/assets/file_$i.bin" bs=$(shuf -i 1-2000000 -n 1) count=1; done && dfx deploy hello_frontend

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@sesi200 sesi200 marked this pull request as ready for review October 9, 2024 15:30
@sesi200 sesi200 requested a review from a team as a code owner October 9, 2024 15:30
@chenyan-dfinity
Copy link
Collaborator

chenyan-dfinity commented Oct 9, 2024

Is icx-asset updated as well? I tested this with one of the caffeine generated directory: https://github.com/chenyan-dfinity/asset-test/tree/main/1723158607637/frontend/dist

$ time ~/src/sdk/target/release/icx-asset upload br5f7-7uaaa-aaaaa-qaaca-cai dist/

On master, it takes 3.475s. On this branch, it takes 3.833s. (Both using wasm from this branch)

My simple chunk upload binary takes 2.158s. I expect icx-asset to be a bit slower because I didn't compute hash and didn't provide gzip files. But at least, it should be faster than master?

@sesi200
Copy link
Contributor Author

sesi200 commented Oct 10, 2024

Is icx-asset updated as well?

Yes, it's 'just' a wrapper around ic-asset.

Just tested myself. TLDR: it speeds things up significantly, but only with enough files. Measurements:

Local with dfx start --clean --artificial-delay 500 since I ran out of ICP to test on mainnet. Should be simulating a subnet with no load and 2 blocks per second, which should be close-ish to current ideal mainnet performance.

Project: dfx new, react frontend
icx-asset-pr:       5.38s
icx-asset-master:   5.35s

Project: dfx new, react frontend, plus 500 random files between 0 and 20k bytes, same files for both runs
icx-asset-pr:       8.45s
icx-asset-master:  34.14s

After verifying with debug prints in the asset canister I am pretty certain this happens:

icx-asset-master, hello world:

  1. round: create_batch
  2. round: 5 parallel calls to create_chunk, all executed in the same round (debug prints show same timestamp)
  3. round: commit_batch

icx-asset-pr, hello world:

  1. round: create_batch
  2. round: create_chunks with 5 chunks in it
  3. round: commit_batch

icx-asset-master, hello world + 500 files:

  • round 1: create_batch
  • then: 500 times create_chunk, up to 25 in parallel. Processes ~20 calls per round
  • last 3 rounds: commit_batch since dfx only commits so many changes at a time (execution limits), all wait for the previous call's response

icx-asset-pr, hello world + 500 files:

  • round 1: create_batch
  • round 2: three calls to create_chunks processed in parallel, all 500 files uploaded
  • round 3-5: commit_batch since dfx only commits so many changes at a time, all wait for the previous call's response

@sesi200 sesi200 merged commit f313af3 into master Oct 11, 2024
297 checks passed
@sesi200 sesi200 deleted the SDK-1769-upload-batched-chunks branch October 11, 2024 13:08
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.

3 participants