-
Notifications
You must be signed in to change notification settings - Fork 15
Zlib compress all wasm files and decompress them during prefetch #170
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for webkit-jetstream-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Very cool! I'll leave some detailed comments on the PR next, but responding first to your question 1:
No, I don't think that's worth the hassle. Some quick data / experiment: I copied all
and compared different compression methods:
I don't think those small savings of a better algorithm / library are worth adding another dependency for. (And we also could no longer use |
Regarding the other points (including Camillo's):
+1 to staying in the JavaScript/npm ecosystem. Would be happy to provide a port / alternative to
One reason for this change was to make the repository smaller on disk (excluding
I agree that it's convenient to have a single script to decompress everything (in particular given the next point by Camillo). But I would like the build scripts to be self-contained / a single step; otherwise I think it's easy to forget or at least annoying having to run another
Agreed; right now compression always forces blob URLs. How about disabling decompression and stripping Line 85 in 5be6cdc
|
Based on `compress.py` from WebKit#170, with some modifications: - Can be run as `npm run compress` or simply `node compress.mjs` - Uses best zlib compression setting. - Takes arbitrary glob patterns for input files, defaulting to all .z files for decompression. - Copies the file mode over to avoid spurious git diffs.
Will also be useful for WebKit#170
Will also be useful for WebKit#170
|
||
// If we aren't supposed to prefetch this and don't need to decompress it, | ||
// then return code snippet that will load the url on-demand. | ||
let compressed = isCompressed(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const compressed = ...
return `load("${url}");` | ||
|
||
if (this.requests.has(url)) { | ||
return this.requests.get(url); | ||
} | ||
|
||
const contents = readFile(url); | ||
let contents; | ||
if (isCompressed(url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use compressed
from above.
help='Decompress all .z files in current directory and subdirectories') | ||
parser.add_argument('--keep-input', action='store_true', | ||
help='Keep input files after processing (default: remove input files)') | ||
parser.add_argument('--directory', default='.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency with find
and the likes, I would have expected this to be a positional argument, i.e.,
parser.add_argument('directory', nargs='?', default='.',
help='Directory to search for files (default: current directory)')
|
||
// Fallback for shell environments without TextDecoder. This only handles valid | ||
// UTF-8, invalid buffers will lead to unexpected results. | ||
function decodeUTF8(int8Array) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use the shared polyfill in #173 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use the node script from #172 instead (which stays in the NPM ecosystem, uses best zlib compression ration, copied file mode over).
@@ -1161,7 +1273,7 @@ class GroupedBenchmark extends Benchmark { | |||
await benchmark.prefetchResourcesForBrowser(); | |||
} | |||
|
|||
async retryPrefetchResourcesForBrowser() { | |||
async retryjForBrowser() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: intended naming change?
Thanks for the reviews! I like the idea of using node for the compression script, will use #172 once it has merged. And also the shared polyfill for TextDecoder.
Yeah that seems like a better path than just silently re-enabling prefetching for those files. I'll implement that.
As long as the uncompressed files are not used by the default runner, it is fine for them to be checked in. I can exclude them when vendoring in the JS3 repo into Firefox, and only copy over the .z files. But also it does seem nice to only have one canonical version of things. What might change this is if we wanted to compress JS files too (which can be diff'ed and inspected easily). From #154, there were three large JS files (excluding tfjs which is disabled) that could be good candidates for this:
How do folks feel about compressing JS too? If that's okay with folks, then we probably should keep the uncompressed versions around.
That's fine with me too. I was just running out of time on Friday and wanted to have something quicker. Updating all the build scripts probably isn't too bad. |
Thanks for kicking this off 👍 +1 on compressing large JS files too – given that this would just work transparently with prefetching! |
#172 landed, so feel free to use / rebase this on top of it.
Edit: Re-reading/thinking about the arguments, I am not so sure about compressing source JS files any more. (JS files that are just like blobs, generated, and won't be manually modified, e.g., inputs for babel are fine to compress.) Keeping uncompressed JS source files around for diff/code review/maintenance sounds like a good idea. In terms of transfer size during loading, there won't be any benefit to compressing in the repo, since a competent web server will use some compression scheme anyway. E.g., the Netlify preview uses brotli (see screenshot, ~2MB vs ~12MB uncompressed for waypoints.js) ![]() |
Here's an alternative idea. What if we just left all of the files in this tree uncompressed, and only added support to JetStreamDriver for decompressing? It would then be up to anyone vendoring the tree to compress whatever files they want and rewrite the paths in the driver. I can have a script that does this as part of the mozilla vendoring process. We wouldn't need to update any build scripts, or do anything for disablePrefetching+compression (we wouldn't be doing that on the vendored copy). I probably could drop all the shell polyfilling for zlib too because we only would be running the vendored copy in the browser. We'd also continue to get good diff's for free. |
Partially fixes #154. A full fix would target some JS files. Opening to get early thoughts on it, there are some integration questions.
.z
files using zlib during prefetch. If prefetch is disabled, these files are still prefetched to ensure the decompression time is outside of the score. In the browser this uses DecompressionStream. In the shell this uses the zlib-wasm code to decompress the file.Open questions:
compress.py
script can automatically decompress all the files in the tree for anyone who wants to read the build artifacts.