-
-
Notifications
You must be signed in to change notification settings - Fork 334
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
POC: use Rust for css color parsing #2647
Conversation
1a8d11a
to
807f0ae
Compare
I ran the Bloaty size test manually on Linux. It is reporting a +364% binary size increase. Diff here: https://gist.github.com/louwers/809e971d9ae3459bcff450487c29249c |
Something to consider is platform support by
https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2-without-host-tools There is no automated testing for Rust or the Rust standard library for iOS right now. |
This is correct, but the Tier 2 label is a bit scarier than it sounds. For example, macOS on Apple Silicon is also somewhat infamously Tier 2 still 😉 It's worth noting that major projects are also using Rust in production on iOS and Android, including Firefox which uses Rust to share code across platforms. |
Can you try building with |
@louwers |
Looking at
I looked at the size increase text dump - looks really weird. It shows a significant increase in all sorts of .cpp files, and I am really not sure why that might be the case. The key changes are in these I think:
Most of these are 1-time cost, i.e. some core mem alloc, utf8, and panic handling, and a 4% increase might be a good trade in exchange for other benefits. But clearly we should pay attention to that. |
repository = "https://github.com/maplibre/maplibre-native" | ||
|
||
[lib] | ||
crate-type = ["staticlib"] |
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 think that for Android (and probably Linux?), you only need a cdylib
. staticlib
is however required for iOS (at the moment).
This is actually somewhat relevant to @louwers' comment about Bloatly too, depending on how Bloatly looks at things, and how smart linking is with JNI on Android. Apologies in advance for any ignorance about its methodology in advance, but the final size of the libraries does not necessarily go straight to release binary size of an application.
Some numbers... Doing a release build of the xcframework for iOS isn't a fair comparison, since that's compressed and includes all architectures (the final app on the user's phone is uncompressed AFAIK and is "thinned" to remove slices per architecture and dependencies on libs that are already in the base system). If you look inside the XCFramework for Ferrostar, you'll find the ios-arm64
folder is 22.1MB. The total reported binary size for a non-trivial app running on my iPhone, which includes MapLibre Native, is only 15.MB. It's one of the smallest apps on my phone (a Debug build is, for reference, only slightly larger at ~19MB) 😂
Android appears to be slightly heavier (screenshot at the end of the post), but slicing the bundle per arch should make things quite manageable. Also notable that his is a debug build; couldn't find an easy way to get Android to generate a release build without a dance for signing.
The point being, it's not adding much to a release binary, even if the library sizes may look a bit scary at first. For contrast, here are the sizes of the most popular apps on my phone: Signal (134MB), LinkedIn (367MB), Gmail (502MB), Slack (392MB), Uber (412MB), CapitalOne (480MB), WhatsApp (197MB), AirBnB (220MB)...
I am not 100% sure that the build settings, Bazel integration, etc. are optimal for this PR yet, but I am confident based on experience with Ferrostar that we should be able to manage the impact to binary size. Rust does make some tradeoffs (all Rust deps must be statically linked), but I expect we will be able to manage the impact to release builds.
TL;DR - 1) let's try building a cdylib
for specific platforms, and 2) let's fact check Bloatly against what it does to an actual release binary for a demo app on several target architectures.
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.
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 think the Rust library is linked with the rest of maplibre to produce a final shared library. So the target of the Rust library should be staticlib for both platforms I believe.
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 agree - for the core features, should always be statically linked with the rest of the lib. For more complex stuff (optional), we might be ok with having multiple binary files
On Small addendum: |
Thx @ianthetechie! Why would we want to build a |
Sorry I may have missed some of which platforms we're targeting / how it's built @nyurik ;) To distribute Rust code for Android via the NDK + Java bridges (JNI/JNA), you usually build a I regrettably don't have more details on why that's the case / what technical limitations there are, but that's what all the tooling bridging cargo and the NDK requires of crates. Maybe it doesn't apply to us since we're essentially linking up a library that will look like just a regular lib with headers and a C ABI already; I guess Bazel is driving a lot of this linking and by the time it gets to our NDK step, it's all indistinguishable anyways. |
Ah, gotcha - yes, the resulting build target that wraps all Rust + C++ functionality could be dynamic - but that's up to the cmake/bazel/... to create and use in JNI. It would not affect how I build the low-level core components that get linked in. Otherwise you end up with JNI -> C++ cdylib -> Rust cdylib. As a separate project, I will try to wrap the whole ml-native as a rust lib. That target will need to support both rustlib and cdylib output. |
@louwers To put the T2 target concern to rest, I pinged a few ppl on Mastodon to get an answer closer to the source, and Esteban Küber, a member of the compiler team responded: https://hachyderm.io/@ekuber/112841995275142925. TL;DR we can rely on stable channel Rust releases; just not nightly (which nobody is proposing here haha). It's more a reflection of CI resources than anything (and as such, perhaps unsurprisingly, eventually x86 macOS will eventually move to T2). |
fe59cf4
to
cc1222f
Compare
This PR has been rebased on the new docker implementation - so now it can be tried very easily without installing anything locally, while still not having to re-download anything on each |
50205f2
to
66d5d04
Compare
for more information, see https://pre-commit.ci
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.
a few minor nits
@@ -200,6 +200,10 @@ jobs: | |||
xvfb \ | |||
x11-xserver-utils | |||
- name: Install cxxbridge-cmd (compile) | |||
if: ${{ github.ref == 'refs/heads/rust' || github.event.pull_request.base.ref == 'rust' }} | |||
run: cargo install cxxbridge-cmd |
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.
run: cargo install cxxbridge-cmd | |
uses: baptiste0928/cargo-install@v3 | |
with: | |
crate: cxxbridge-cmd |
":rustutils", | ||
], | ||
visibility = ["//visibility:public"], | ||
) |
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.
) | |
) | |
repository = "https://github.com/maplibre/maplibre-native" | ||
|
||
[lib] | ||
crate-type = ["staticlib"] |
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 agree - for the core features, should always be statically linked with the rest of the lib. For more complex stuff (optional), we might be ok with having multiple binary files
set -e | ||
|
||
cxxbridge rustutils/src/lib.rs --header > rustutils/lib.h | ||
cxxbridge rustutils/src/lib.rs > rustutils/cpp/src/lib.rs.cc |
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.
cxxbridge rustutils/src/lib.rs > rustutils/cpp/src/lib.rs.cc | |
cxxbridge rustutils/src/lib.rs > rustutils/cpp/src/lib.rs.cc | |
See #3137 I made some changes. |
Bloaty Results 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2647-compared-to-main.txtCompared to d387090 (legacy)
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2647-compared-to-legacy.txt |
Benchmark Results ⚡
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2647-compared-to-main.txt |
Bloaty Results (iOS) 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2647-compared-to-main.txt |
This is a totally incomplete proof of concept how a Rust css parser can be used from C++.
This PR uses changes from #2643
rustutils
crate is the entry point for all rust utilitiesvendor/csscolorparser
-- to ensure we don't accidentally rely on itHelp needed