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: browser chunking #11102

Merged
merged 23 commits into from
Jan 9, 2025
Merged

feat: browser chunking #11102

merged 23 commits into from
Jan 9, 2025

Conversation

Thunkar
Copy link
Contributor

@Thunkar Thunkar commented Jan 8, 2025

Further modularises noir-protocol-circuits-types, exposing a client/lazy API that allow browser bundles to not import every single JSON artifact at once, but rather pull them on demand. /server and /vks entrypoints are also provided to aid in startup CLI times, jest cache and general cleanup.

This is very important due to the sheer amount of different reset variants we currently have. However, a regular /client/bundle is still exposed in case dynamic imports are not supported in the target platform.

This takes our bundle sizes in the example vite box (with PXE in the browser and proving) from 50MB (30 compressed) to ~3MB before meaningful content is displayed. After that, bb.js is an additional 8-9 and each circuit hovers around 0.3-1, but only the ones used in the transactions are pulled when required.

Also removed the TestPrivateKernelProver, and replaced it by each of our prover implementations providing a cleaner simulation vs. witgen+proving interface. This way, the prover is now 100% a PXE plugin (which will allow us to even delegate proving!)

@Thunkar Thunkar self-assigned this Jan 8, 2025
@Thunkar Thunkar added e2e-all CI: Enables this CI job. network-all Run this CI job. labels Jan 8, 2025
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Looks good! I left several comments but none of them are blockers. More in general, I got the same concern as with the previous PR for browser usage: it's really easy to screw this up by adding the wrong import in the wrong place. Should we tweak the vite or webpack box config so that it fails with an error if the bundle exceeds a certain size, so we don't inadvertently screw up the bundle again?

And a nit about naming: I'm not a fan of server/client/client-async. WDYT about server/client/browser? Though server/client can mean "the artifacts for the server/client" or "to be used in the server/client", which makes things confusing. Hmm...

@Thunkar Thunkar marked this pull request as draft January 8, 2025 15:26
@Thunkar
Copy link
Contributor Author

Thunkar commented Jan 8, 2025

Thanks for the comments @spalladino !

Had to revert to draft because I wanted to rework TestPrivateKernelProver too and completely forgot 😓

  • Regarding moving stuff to base classes: totally agree and will try. But the tricky part is not polluting the imports, and it can get complicated quick with things sneaking in.
  • Naming: Not 100% convinced about /browser since node can also load asynchronously (and it might make sense, eg: the CLI). Also not very fond of /client-async either.
  • Tracking of bundle sizes: again 100% agreed, but wanted to wait until CI3 stabilization before adding a new task

@spalladino
Copy link
Collaborator

Tracking of bundle sizes: again 100% agreed, but wanted to wait until CI3 stabilization before adding a new task

I don't think it needs to be a new task. We should be able to configure webpack on aztec.js to error on bundle size exceeded using performance.hints and maxAssetSize (see here), and that build is already happening on CI.

@spalladino
Copy link
Collaborator

As for naming, I asked ChatGPT. I like the bundle vs lazy terminology:

Here are a few naming suggestions for your export entrypoints, keeping clarity and conventions in mind:

Option 1: Descriptive and Clear

"exports": { "full": "./full.js", "lazy": "./lazy.js" }
"exports": { "all": "./all.js", "async": "./async.js" }
"exports": { "eager": "./eager.js", "lazy": "./lazy.js" }

Option 2: Functional Names

"exports": { "importAll": "./import-all.js", "loadOnDemand": "./load-on-demand.js" }
"exports": { "preloaded": "./preloaded.js", "dynamic": "./dynamic.js" }
"exports": { "direct": "./direct.js", "asyncLoader": "./async-loader.js" }

Option 3: Conventional with API Terminology

"exports": { "static": "./static.js", "dynamic": "./dynamic.js" }
"exports": { "bundle": "./bundle.js", "loader": "./loader.js" }
"exports": { "synced": "./synced.js", "async": "./async.js" }

Option 4: Minimalist Style

"exports": { "sync": "./sync.js", "async": "./async.js" }
"exports": { "all": "./all.js", "lazy": "./lazy.js" }
"exports": { "core": "./core.js", "loader": "./loader.js" }

Key Considerations:

Eager/static/full: Names for the entrypoint that imports all artifacts directly.
Lazy/dynamic/async: Names for the entrypoint that asynchronously loads artifacts.
Consistency in naming (if you use verbs for one, like load, use them for both entrypoints).

@Thunkar Thunkar marked this pull request as ready for review January 9, 2025 06:59
@Thunkar
Copy link
Contributor Author

Thunkar commented Jan 9, 2025

@spalladino implemented your suggestions, finally went with /client/lazy and /client/bundle (with an additional /client for the common stuff)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome

@Thunkar Thunkar merged commit 393e843 into master Jan 9, 2025
78 checks passed
@Thunkar Thunkar deleted the gj/browser_chunking branch January 9, 2025 13:19
TomAFrench added a commit that referenced this pull request Jan 9, 2025
* master: (287 commits)
  feat: Sync from noir (#11051)
  chore(docs): Update tx concepts page (#10947)
  chore(docs): Edit Aztec.nr Guide section (#10866)
  chore: test:e2e defaults to no-docker (#10966)
  chore(avm): improve column stats (#11135)
  chore: Sanity checking of proving job IDs (#11134)
  feat: permutation argument optimizations  (#10960)
  feat: single tx block root rollup (#11096)
  refactor: prover db config (#11126)
  feat: monitor event loop lag (#11127)
  chore: Greater stability at 1TPS (#10981)
  chore: Jest reporters for CI (#11125)
  fix: Sequencer times out L1 tx at end of L2 slot (#11112)
  feat: browser chunking (#11102)
  fix: Added start/stop guards to running promise and serial queue (#11120)
  fix: Don't retransmit txs upon node restart (#11123)
  fix: Prover node aborts execution at epoch end (#11111)
  feat: blob sink in sandbox without extra process (#11032)
  chore: log number of instructions executed for call in AVM. Misc fix. (#11110)
  git subrepo push --branch=master noir-projects/aztec-nr
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job. network-all Run this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants