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

rewrite bundling for smaller bundles #3

Merged
merged 3 commits into from
Sep 12, 2019

Conversation

devsnek
Copy link
Contributor

@devsnek devsnek commented Sep 11, 2019

  • Remove redundant async functions in common cases
  • Replace validation with smaller call to WebAssembly.validate
  • Remove duplicated check for shared memory
  • Removes fetch(data uri) to avoid CSP.

Overall, this results in ~6% size savings (621 bytes vs 587 bytes

@surma
Copy link
Collaborator

surma commented Sep 11, 2019

So I like where this is going, thanks for your effort!

Two things:

  1. The index.js files contained // Name: and // Proposal: comments that are used to programmatically update the readme. This would need to be moved to the .wat file.
  2. This is actually bigger after gzip (621 bytes on master vs 686 bytes on this branch). I think you might be better off not trimming the wasm preamble as gzip will take care of that.

An idea I’ve had (and maybe you wanna take a stab at that): I wondered if hex encoding is better.

  • The decoder is literally new Uint8Array(hexstring.split(/(..)/).filter(x => x).map(v => parseInt(v, 16)))
  • Same code works in node and web
  • It probably compresses just as well as base64 with gzip

WDYT?

@devsnek devsnek force-pushed the smaller-bundles branch 2 times, most recently from 058c739 to ddac08e Compare September 11, 2019 19:07
@devsnek
Copy link
Contributor Author

devsnek commented Sep 11, 2019

@surma what are you using to test the gzipped size. by my count we are now saving exactly 1 byte, but we also no longer rely on fetch.

@devsnek devsnek force-pushed the smaller-bundles branch 2 times, most recently from 8cd34d3 to 0158149 Compare September 11, 2019 19:18
@surma
Copy link
Collaborator

surma commented Sep 11, 2019

cat dist/esm/index.js | gzip -c9n | wc -c

@devsnek
Copy link
Contributor Author

devsnek commented Sep 11, 2019

in that case we're now down to 587

@surma
Copy link
Collaborator

surma commented Sep 11, 2019

Brilliant! Thanks a bunch!

I’m on mobile for today, but I’ll give this a proper review tomorrow 🎉

@devsnek devsnek force-pushed the smaller-bundles branch 2 times, most recently from da6cd09 to a7faa45 Compare September 11, 2019 19:52
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@@ -0,0 +1,12 @@
import { gzipSync } from "zlib";

export default function sizePrinter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good thinking on this one :D

@surma
Copy link
Collaborator

surma commented Sep 12, 2019

I removed the global data object as it would make the code not as tree-shakable. We are also down to 560 Bytes :D

If you have on objections, I’ll merge this :)

@surma
Copy link
Collaborator

surma commented Sep 12, 2019

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@surma
Copy link
Collaborator

surma commented Sep 12, 2019

... lol

@surma surma merged commit 8c528be into GoogleChromeLabs:master Sep 12, 2019
@devsnek devsnek deleted the smaller-bundles branch September 12, 2019 16:36
@devsnek
Copy link
Contributor Author

devsnek commented Sep 12, 2019

really nice thinking on the bind(null, hex string)!

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.

None yet

3 participants