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

Rust crate #158

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Rust crate #158

wants to merge 7 commits into from

Conversation

simoncozens
Copy link
Contributor

Provides a Rust interface to the language data. Uses the data in the Lib/gflanguages/data directory, compiling it into Rust lazy-static structures at build time.

Cargo.toml Outdated Show resolved Hide resolved
@simoncozens
Copy link
Contributor Author

simoncozens commented Sep 4, 2024

Okay, I'm having second thoughts about this. The way that we're building up the data is "clever", and every time I do something "clever" I should stop and think if it's actually the right thing. Originally my plan was to store the data as data, ie. in the .text section, but then I hit a wall because (a) you can't store a BTreeMap as a static constant, and (b) the string data needed to be Strings to match the prost types, so we needed to call .to_string(), so suddenly all the data has to be code. And storing lots of data as code loses all the benefit of having it in .text and leads to horrible bloat and compile times and smashed stacks and other problems. But I thought "great, if the data is all built up in code we don't need to do protobuf parsing at runtime, win win win" and I was so busy trying to solve the smashed stacks I didn't think about the other problems.

So now I'm thinking that just storing all the data as a big serialisable thing and deserialising it at runtime is cleaner. Don't know, will try it.

@madig
Copy link

madig commented Sep 4, 2024

FWIW, I hit a similar compilation times problem in https://github.com/madig/glyphsinfo-rs when I tried to include all the GlyphInfo.xml data as plain old data types to compile. Or maybe it was me trying to use enums where it made sense. I then settled on using https://crates.io/crates/postcard to include a binary dump of the data to deser at runtime 🤷‍♀️

@simoncozens
Copy link
Contributor Author

I used serde_json here for debuggability and I'm agnostic about moving to other serialisation formats. In view of Rod's low-dependency mantra I figure if we're going to have a dependency anyway it may as well be a familiar one rather than an exotic one.

Moving to deserialisation knocked 15% off the binary size of fontspector-wasm, so yeah, much better than building the data in code.

@RickyDaMa
Copy link

RickyDaMa commented Sep 4, 2024

you can't store a BTreeMap as a static constant

Maybe using phf would be of interest, its OrderedMap sounds close to what you're after: https://docs.rs/phf (no sorting on creation/insertion though).

This doesn't solve the problem to the point of you being able to write everything in Rust source code instead of deserialising at runtime, but it's part of it I guess. I've not messed with prost types before but could they use Cow<'static, str> instead of String? Then you can have your static data at compile-time or runtime

@simoncozens
Copy link
Contributor Author

I've not messed with prost types before but could they use Cow<'static, str> instead of String?

Sadly not:

            Type::String => format!("{}::alloc::string::String", prost_path(self.config)),

https://github.com/tokio-rs/prost/blob/2fc71325db4a7abfe85ac42f7b55be87160fc325/prost-build/src/code_generator.rs#L960

Anyway I think I'm pretty happy with where I've got to.

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