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

Use -Oz instead of -O2 to reduce compiled asset sizes significantly #479

Merged
merged 5 commits into from
Jun 1, 2022

Conversation

Taytay
Copy link
Contributor

@Taytay Taytay commented Aug 29, 2021

(Based on PR #478, so the diff of this is actually minimal - just a change to using -Oz instead of -O2)

This reduces the compiled size of most assets by about 50% :
image

To specify the result I'm happiest about: The .wasm file went from 1,108,328 bytes to 593,151 bytes.
Zipped it went from 442,420 bytes to 298,116 bytes.
(I included the .zip version of each file to represent what this is more likely to look like across the wire.)

For details on optimization settings: https://clang.llvm.org/docs/CommandGuide/clang.html
We were previously running with -O2: -O2 Moderate level of optimization which enables most optimizations.
This switches to -Oz: -Oz Like -Os (and thus -O2), but reduces code size further.

In rudimentary perf testing I did (long, long ago), it was also fast (as fast as the -O2 option as I recall). However, at the time, I noted that the -Oz file took 4x as long to load the .wasm file as the -O2 file did.

Out of date notes on my experiments here in the Makefile: https://github.com/Taytay/sql.js/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R213-R378

Before we merge this, I think we need a better benchmarking script to ensure we aren't about to doom everyone to a massive perf regression. I can dig up my old script as a starting point, or perhaps we can use what jlongster wrote to benchmark: #447 (comment)

If anyone has a suggestion on a benchmarking script to use, I'm all ears.

On a slightly related note, I would also like to improve our default SQLite compilation options to adopt most of these official recommendations: https://www.sqlite.org/compile.html
That slightly improves file sizes and runtimes as well. However, we should do that in a separate PR, as that has the greater chance of breaking existing deployments.

This reduces the chance that another dev will forget to do this.
Changes required:
* Defined EM_NODE_JS environment variable to get rid of
a warning that appears if the NODE environment variable
is set, but EM_NODE_JS is not.
* EXTRA_EXPORTED_RUNTIME_METHODS is now EXPORTED_RUNTIME_METHODS
* No longer pass the `-s LINKABLE=1` option to emcc when compiling. (This is a linktime setting and emcc
warns about not using a linktime setting when compiling now).
Sqlite now publishes the hash as a SHA3 instead of SHA1,
necessitating the installation of the sha3sum command.
This reduces most compilation output by around 50%
@kaizhu256
Copy link
Member

kaizhu256 commented Aug 30, 2021

On a slightly related note, I would also like to improve our default SQLite compilation options to adopt most of these official recommendations: https://www.sqlite.org/compile.html

note the recommended-compile-option SQLITE_USE_ALLOCA might be worse for both filesize and performance according to this article - http://troubles.md/the-stack-is-not-the-stack/

but wow, what was reported is awesome

@lovasoa
Copy link
Member

lovasoa commented Aug 30, 2021

These results are very encouraging ! Let's run a performance tests, and if there is no significant performance degradation, let's merge !

@Taytay
Copy link
Contributor Author

Taytay commented Aug 30, 2021

Thanks for that warning @kaizhu256! I didn't realize that, and will definitely keep that in mind!

@rhashimoto
Copy link

I just experimented with switching from -O3 to -Oz in my own SQLite project where the WASM build behavior should be nearly identical. My .wasm size improved from 987964 to 495761, so comparable to what you have.

I have my own benchmark page, and I have attached results for both builds on my local machine. The "default" column would be closest to the expected SQL.js results. Neither is consistently faster so I think most of the difference is just noise.

So I concur that using -Oz yields pretty substantial space savings with little to no performance cost on essentially the same code base.

benchmarks -O3.pdf
benchmarks -Oz.pdf

@Taytay
Copy link
Contributor Author

Taytay commented May 3, 2022

Nice! This set of benchmarks, or something like it, was exactly what I wanted to see. Thanks for putting it together. This meets the bar as far as I'm concerned. (50% space savings, near-equivalent perf).
I motion to merge it. @lovasoa - would you be up for clicking the button? (On a related note, I am happy to help maintain as well, and there might be some others here that are interested?)

@Taytay Taytay marked this pull request as ready for review May 3, 2022 20:57
@lovasoa lovasoa merged commit adb788c into sql-js:master Jun 1, 2022
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.

4 participants