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(avm)!: store public bytecode compressed in artifact #11193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fcarreiro
Copy link
Contributor

@fcarreiro fcarreiro commented Jan 13, 2025

Store public bytecode compressed in the artifact JSON, but uncompress when loading.

Closes #11150.

@fcarreiro fcarreiro requested a review from dbanks12 as a code owner January 13, 2025 18:09
Copy link
Contributor Author

fcarreiro commented Jan 13, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@fcarreiro fcarreiro changed the title feat(avm)!:store public bytecode compressed in artifact feat(avm)!: store public bytecode compressed in artifact Jan 13, 2025
@fcarreiro fcarreiro removed the request for review from dbanks12 January 13, 2025 18:10
Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor Author

Not sure if this will fly if it gets used in the browser. @Thunkar ?

Copy link
Contributor Author

Yeah indeed

 ERROR in ../types/dest/abi/contract_artifact.js 3:0-34
  Module not found: Error: Can't resolve 'zlib' in '/build-volume/yarn-project/types/dest/abi'
  
  BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
  This is no longer the case. Verify if you need this module and configure a polyfill for it.
  
  If you want to include a polyfill, you need to:
  	- add a fallback 'resolve.fallback: { "zlib": require.resolve("browserify-zlib") }'
  	- install 'browserify-zlib'
  If you don't want to include a polyfill, you can use an empty module like this:
  	resolve.fallback: { "zlib": false }

Gzipping reduces the public bytecode size ~50%. It would probably pay off to add a polyfill. WDYT?

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.

LGTM!

Note that this will make ts code startup slower, since loadContractArtifact gets called at the top-level in several files. So just importing a file that makes this call will incur on the gzip cost. Not sure how much it is, but if it's noticeable (especially when multiplied by all the contracts we have) we may want to make this lazy and hide it behind a property getter.

Another option would be to not compress artifacts at all, and instead rely on native browser compression (assuming the main issue here is time to download artifacts), but we'd need to check how it compares to the best compression we're using.

Apologies for not catching this during earlier discussions, I only noticed it when I saw the gunzipSync call written down.

Copy link
Contributor Author

fcarreiro commented Jan 14, 2025

we may want to make this lazy and hide it behind a property getter.

That's a good idea, will check it out.

and instead rely on native browser compression

Yeah this was kind of my point in the slack discussion. I think even if we compress like in this PR, browser compression will squeeze some gains over the base64 encoding etc.

I discussed with @Thunkar and he pointed me to the pako import that's browser friendly and being used in some other places already.

I'll consider all this but it might just be simpler to not compress and go with something like #11101 which would already reduce public bytecode 50%.

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.

Compress public functions in json artifact, but still deploy uncompressed
3 participants