Skip to content

Replace ourboros with self_cell in ftml #270

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

Merged
merged 1 commit into from
May 10, 2021
Merged

Replace ourboros with self_cell in ftml #270

merged 1 commit into from
May 10, 2021

Conversation

Voultapher
Copy link
Contributor

@Voultapher Voultapher commented May 9, 2021

Before:

$ cargo build --target wasm32-unknown-unknown
... 161 dependencies
Finished dev [unoptimized + debuginfo] target(s) in 1m 53s

After:

$ cargo build --target wasm32-unknown-unknown
... 152 dependencies
Finished dev [unoptimized + debuginfo] target(s) in 1m 33s

That's 22% faster in this very rough test. But the 9 fewer dependencies make this seem plausible.

There are still some proc macro dependencies like syn left, but they have nothing to do with self_cell as it has 0 dependencies and doesn't use proc macros.

Disclaimer I'm the author of self_cell.

I would have like to run the tests before and after, but I don't know how to do that for wasm. Please check this works like before. That said. the heavy lifting is done in the type system here and I expect no runtime problems.

@Voultapher Voultapher requested a review from emmiegit as a code owner May 9, 2021 17:59
@Voultapher Voultapher changed the title Replace ourboros with self_cell Replace ourboros with self_cell in ftml May 9, 2021
@emmiegit
Copy link
Member

emmiegit commented May 9, 2021

Yeah, I created a proc macro for building a logger-free version of the library, since logging is a pretty notable performance hit on wasm. Thank you for bringing up this crate, I will take some time and look into this crate as an alternative.

@emmiegit
Copy link
Member

After looking at this for a bit, I think this is fine. I'll merge it.

@emmiegit
Copy link
Member

@Voultapher, I'm curious, how did you find this repo?

Copy link
Member

@Monkatraz Monkatraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I packed this PR up and ran the client/ tests and the results were interesting: At first, I thought there was a serious performance regression - something like 5x slower. However, goofy V8 compile caches and web browser console shenanigans struck, and led me astray.

It seems that this has a performance benefit in browser - about 25ms quicker for SCP-1730, ~150ms to ~125ms. One interesting "regression" is that the WASM binary size increased quite a bit - from ~1750KB to ~2100KB. Not sure why that is, but this binary was already large so I'm not sure if I mind it that much.

It does pass all WASM tests, so no issue there.

@Voultapher
Copy link
Contributor Author

@Voultapher, I'm curious, how did you find this repo?

Initially I developed this crate for a private use case, but the more it evolved I realized it could replace many ouroboros use cases, so I took a look at the dependents of ouroboros https://crates.io/crates/ouroboros/reverse_dependencies and started opening PRs.

@Voultapher
Copy link
Contributor Author

It seems that this has a performance benefit in browser - about 25ms quicker for SCP-1730, ~150ms to ~125ms.

Glad to hear it is also better at runtime performance. This is good data to confirm my expectation, that the two performance critical changes I made with the rework help address Voultapher/self_cell#2.

One interesting "regression" is that the WASM binary size increased quite a bit - from ~1750KB to ~2100KB. Not sure why that is, but this binary was already large so I'm not sure if I mind it that much.

The macro only compiles in things it needs and that are asked for, so the only thing I can think of is the crate being no_std and having to use extern crate alloc, please try out this branch where I made the crate not no_std and report back your findings. Add self_cell = { git = "https://github.com/Voultapher/self_cell", branch = "no-no-std" } to your Cargo.toml.

@Monkatraz
Copy link
Member

Monkatraz commented May 10, 2021

The macro only compiles in things it needs and that are asked for, so the only thing I can think of is the crate being no_std and having to use extern crate alloc, please try out this branch where I made the crate not no_std and report back your findings. Add self_cell = { git = "https://github.com/Voultapher/self_cell", branch = "no-no-std" } to your Cargo.toml.

Alright, so I figured it out. We have to vendor in binaries for our client-side FTML module for the time being, and I apparently hadn't built them in a while and we haven't had a version bump, so they never got reported as out of date.

Having properly compared the two builds now, the actual size difference is... ~1KB. We do compile the WASM with the maximum number of optimizations possible (although not for code size), so it probably makes sense that the code is barely different.

Sorry for reporting that wrong - although, I'll quickly mention how you can test the client-side WASM if you want to, assuming you have Node and NPM:

$ cd client/
$ npm install && npm run bootstrap
$ cd modules/ftml-wasm
$ npm run pack-ftml # or npm run pack-ftml-debug
$ cd ../../         # back to root
$ npm test          # runs all tests

You'll have to repack if you make changes. The built files will be found in client/modules/ftml-wasm/vendor.

@Monkatraz
Copy link
Member

The binary size bump is pretty spooky now that I think about it - the only PR that could've done it is #266. And I have no idea why that would've done it - @ammongit, any ideas what's going on there?

@emmiegit
Copy link
Member

thanks @Monkatraz @Voultapher

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.

3 participants