-
Notifications
You must be signed in to change notification settings - Fork 30
Reduce CppInterOp Emscripten shared library size #655
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
Reduce CppInterOp Emscripten shared library size #655
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
=======================================
Coverage 79.68% 79.68%
=======================================
Files 9 9
Lines 3943 3943
=======================================
Hits 3142 3142
Misses 801 801 🚀 New features to boost your workflow:
|
It's a compile time optimization flag I think. Do we need to force it like this everywhere or can we have cmake handle this ? |
It is both a link and compile time optimisation. See https://emscripten.org/docs/tools_reference/emcc.html#emcc-oz . |
ee75d01
to
d9f6630
Compare
Yeah but we still don't want users to look into/pass flags correct during the build correct? We should be able to have cmake handle this. |
I really don't understand the point your trying to make. Using EMCC_CFLAGS in this way to perfectly legitimate way of doing the Emscripten build. Emscripten is designed to be effected by modifying this flag (see https://emscripten.org/docs/tools_reference/emcc.html#environment-variables). If a user changes what we have in the documentation for this flag, and messes something up, then that is on them. I don't understand how putting it in a cmake option changes that. |
My point is simple. We already have compile time and link time flags being passed through cmake, I don't see a need to pass this externally. |
I think what @anutosh491 means is that we need to make this flag as part of our default compiler flags in the CMakeLists.txt. |
@vgvassilev @anutosh491 I first tried locally using |
Putting a note here in case I forget. Using EMCC_CFLAGS="-flto" in the llvm build doesn't end up effecting the shared library size on its own. Used in conjuncation with Oz then the size falls slightly to 45/46 Mb. |
db280e0
to
de7101c
Compare
I have updated this PR to use the flto flag as well, and to compile+link CppInterOp shared library with these flags too. This reduces the shared library size to 44/45 Mb. |
6869da8
to
effdaf8
Compare
@vgvassilev I have now changed this PR to a cmake only solution, instead of using EMCC_CFLAGS. Hopefully that means this PR is ready to go in once the ci passes. Can you review and approve if your happy? The Emscripten cache will need deleting before merging this PR, so that the deployment gets the new smaller library. |
effdaf8
to
ba6ed4c
Compare
The failing job is because I accidentally deleted the wrong cache part way through the ci run. Will rerun tomorrow to show the failed job passing. Noticed that with this change, the Emscripten caches are twice the size they were before, so will need to delete Emscripten llvm 19 jobs for this to go in I think. |
@vgvassilev pinging for review |
Can we add some check in the CI to fire when we break this. Maybe something that checks if the file is greater than 50 and fail the build? |
586c2a5
to
3c29fb3
Compare
I have added a check, which on my local system will error if the size is greater than 46 Mb. Need to wait for ci to pass, to check I implemented correctly in the workflow file. |
Hi, please refrain from merging this PR before a set of reviews on my end. Emscripten-forge applies few optimization flags itself (similar to what pyodide does) to add a basic level of optimization for every package while building. So if this goes in (and after a release if we build the latest cppinterop on emscripten-forge) |
I do not see how these flags can go against each other. We have tests that are running and if the tests are good I do not see a reason to hold things off. Right now we destroy people's network by forcing them to download 300mb, ~100 of which is CppInterOp. If this PR cuts the CppInterOp size by half while the tests still run this is something we need urgently, right? |
85cdecc
to
c6f89c0
Compare
Hey @mcbarton can you educate me with some questions
Cause according to my attempts So only making changes to cppinterop might not be good enough, possibly we won't even need to make changes on cppinterop to reduce its size .... which gets us to llvm. Now I am trying to understand what exactly is being optimized through the above flags to get job done cause libclangInterpreter.a which we link against is anyways not too huge (412K on emscripten-forge) though the transitive dependencies being pulled in might be heavy (I see libclang-cpp.a is 147M) |
Hi @anutosh491 on main the size of the shared library is 81 Mb. As far as what the Oz flag is doing under the hooe to reduce size, there doesn't seem to be much information online. The only thing I managed to work out for sure from my testing is that it decreases the inline-threshold flag to near 0. From my recollection, using just the Oz flag during the llvm build (I think using Os reduced it to 60 something), will increase the size of the llvm static libraries, but reduce the size of the shared library to 48 Mb. Adding the flto flag after that reduced the llvm build will reduce its size to 46 Mb. Adding both of these also to CppInterOps compile and link optione reduced it down to 45 Mb. The llvm build is definitely the weak link, but we shouldn't ignore any savings in size we can make on the CppInterOp side too. Of what is left I know that llvm having assertions on is responsible for another 4-5 Mb (but turning them off introduces warnings about unused variables in CppInterOp. @vgvassilev CppInterOps tests can pass without them, so in may be worth having a no assertions deployment alongside an assertions one for debugging issues in a future PR ). Using flto with another 2 flags (I can't remember which ones off the top of my head, and which I haven't currently tried out separately) get us another 1 Mb. I didn't include these 2 flags so far, as I remember not understanding what they were doing from the clang flag descriptions. One possible route forward to further reduce the size is to switch to side_module=2 , where we control what ends up in the shared library (rather than the default which is basically include everything I think). This idea would require quite a lot of thinking though, and shouldn't be part of this PR. |
4e716da
to
22421d2
Compare
@vgvassilev @anutosh491 If I build locally according to the instructions in the readme of this PR, this is the deployment I get https://mcbarton.github.io/xeus-cpp-demo/lab/index.html. Everything which works for me when I follow mains Emscripten instructions works in this deployment too. The 'SIMD Acceleration through WebAssembly' example doesn't work in the link (fails to find wasm_simd128.h), but it doesn't work locally if you build according to the instructions on main (e.g. no modified llvm or CppInterOp). That is the reason I have opened this issue #680 |
With the changes in this PR, the shared library size is down to 39 Mb. The ci has been changed to check whether any future PRs make the library greater than 40 Mb. Hopefully you'll be able to convince yourselves from the link that no performance regressions have happened. |
I have updated the link in this comment with the patch from #686 and the fix to the documentation to get SIMD Acceleration through WebAssembly working (coming in a future PR), so that everything works. |
PRIVATE "SHELL: -Oz" | ||
PRIVATE "SHELL: -flto" |
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.
Can't these become part of CMAKE_X_FLAGS_RELEASE
flags, too?
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.
I will give it a go and find out.
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.
I tried this out and couldn't get it to work. For some reason if I do it via cmake options then Ubuntu both x86 and arm cannot pass xeus-cpps Emscripten tests for llvm 20. I have reverted back to the original solution now, and waiting to see the ci pass
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.
How's that even possible...
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.
Its not supposed to he possible according to the WebAssembly/Emscripten standard. The result should be independent of the platform you cross compile from, but I have come accross a few examples where this is found not to be true in reality (and shows exactly why we should be cross compiling from multiple platforms in the ci to tests these assumptions). In the process of trying to get the cmake approach to work I found out that the symbols you need to export on each platform manually to get the CppInterOps tests to pass are not the same (e.g. you can not export _ZN4llvm15SmallVectorBaseIjE8set_sizeEm , and the Emscripten tests will build and pass on Ubuntu, but wont build on MacOS or Windows)
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.
@vgvassilev Here is a workflow run where Ubuntu arm and x86 fail for llvm 20 Emscripten builds using cmake options https://github.com/compiler-research/CppInterOp/actions/runs/16596946952 despite the other platforms passing
Update CMakeLists.txt Update emscripten.yml Update deploy-pages.yml Update docs Add new exported symbol Revert "Update exports.ld" This reverts commit 26026fe76fa22270252ac3dc377e9fe294678281.
de1dec0
to
a6246f1
Compare
@vgvassilev can this go in now if the ci passes still? The link in this message above shows that there is no performance degradation. If yes, please approve and i'll take care of the merging of the PR. |
The old way of doing it is still able to pass the ci for all jobs. |
Hi, Some remarks based on my comment here (#655 (comment)) One of my questions there was
To be fair, I still don't understand what exactly brings down the size :\ Just thinking about what can bring down the size from 80 to 45
I would have been really happy if we could pin point on what exactly changes in llvm when we us lto (in combination with oz) and how does it get down cppinterop's size as a whole but I guess this is OK. Let's try this approach out and try to backtrack how it helps :) |
Do I read this as a sort of lgtm? |
Yes let's go ahead ! |
@mcbarton can you rebase this PR and squash all changes in a single commit? |
Description
Please include a summary of changes, motivation and context for this PR.
@vgvassilev This change locally reduced the Emscripten shared library size to 47/48 Mb (depends of whether you use dir -lh or du -lh). It still passed all tests and xeus-cpp still could run all the notebook cells it could before (exception throwing is broken in CppInterOps deployment at the moment for some reason). I have cleared the cache of all llvm 20 Emscripten builds for this PR.
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist