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

WIP: Wasm bindings #394

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from
Draft

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Mar 12, 2023

Objective

Fixes #241

Context

@load1n9 has published wasm bindings for Taffy as gelatin, but:

  • There's a bunch of boilerplate and duplicated types that is only necessary because Taffy's types don't have the requisite #[wasm_bindgen] attributes
  • It would be good to offer official first party wasm bindings for Taffy

Notes

  • This is based on the bindings from gelatin which are themselves based on the bindings from stretch.
  • I've removed a bunch of duplicated types by adding #[wasm_bindgen] annotations the main Taffy style types (behind a wasm feature flag) where appropriate (mostly the plain enums). The lib.rs in the bindings crate is down to ~350 lines now, although that will need to increase slightly for grid support.
  • I've moved some of the remaining necessary boilerplate (such as FromStr and TryFrom impls) into the main Taffy crate as these seem generally useful (and are more easily maintained when they live next the type definitions).
  • I tested including the lightningcss based CSS parser from WIP: Add ability to parse a Style struct from a CSS string #393, but that increased the binary size from ~200kb to ~2mb which seems unacceptable to me. So we'll need to use a different API. Luckily an API for most styles is already implemented, but we may need to implement a parser for some of the grid styles.

Tasks

  • Initial import and cleanup
  • Fix the ability to set measure functions
  • Rename the Allocator type to something more friendly
  • Add bindings for CSS Grid styles
  • Enable testing both the main Taffy crate with the wasm feature and the bindings on CI
  • Basic developer documentation
  • Basic end-user documentation
  • Setup publishing to NPM

Feedback wanted

  • Any thoughts on the API design are welcome
  • General review of the implementation
  • Anything that people don't understand about the implementation (and would block them from maintaining it).

@nicoburns
Copy link
Collaborator Author

Hmm... I'm having trouble with the Send + Sync bounds on measure functions. The JS function handle I get from isn't Send, which means I can't store it in the measure func. And a Mutex doesn't even help because that only does Sync not Send!

@load1n9
Copy link

load1n9 commented Mar 13, 2023

Thoughts on deno bindings aswell? should be easy to do.

@nicoburns
Copy link
Collaborator Author

Thoughts on deno bindings aswell? should be easy to do.

I'm very happy to include them so long as:

  • The extra code required is relatively short
  • How the bindings work is well understood / documented such that can be easily maintained going forwards

My understanding is that this shouldn't require any extra Rust code, and it's just the packaging that would differ. So it seems like those requirements should be easy to meet.

I also already want to do somewhat custom packaging so as to make single package that works with both node and web. So this could likely fit quite neatly into that.

@load1n9
Copy link

load1n9 commented Mar 13, 2023

sounds good

@gigasource
Copy link

Hi , i try to create 10000 nodes on browser and it takes 600 ms , i think it is too slow . Yoga wasm takes only 43ms for same task.

@nicoburns
Copy link
Collaborator Author

Hi , i try to create 10000 nodes on browser and it takes 600 ms , i think it is too slow . Yoga wasm takes only 43ms for same task.

Is that using this PR? How are you compiling/running it? Perhaps we ought to expose Taffy::with_capacity as part of the WASM api...

@gigasource
Copy link

yes , i use this pr, i try Taffy::with_capacity too, but the result is the same.

@nicoburns
Copy link
Collaborator Author

yes , i use this pr, i try Taffy::with_capacity too, but the result is the same.

Ok, this will need some investigation. If you have a minimal test script that reproduces the issue that would be helpful.

@gigasource
Copy link

gigasource commented Mar 20, 2023

  const allocator = new Allocator(11000);
  const container = new Node(allocator, {
    position: Position.Absolute,
    width: 500,
    height: 500,
    justifyContent: AlignContent.FlexStart,
    // gapWidth: 50
  });
  container.setStyle({flexDirection: FlexDirection.Row})

  console.time('create1')
  const arr = [];
  for (let i = 0; i < 10000; i++) {
    const child0 = new Node(allocator, {
      flexGrow:1,
      height: "100%"
    });
    container.addChild(
      child0,
    );
    arr.push(child0);
  }
  console.timeEnd('create1')

  const layout = container.computeLayout({});

result: create1: 556.701171875 ms

@gigasource
Copy link

#[wasm_bindgen]
impl Allocator {
    #[wasm_bindgen(constructor)]
    pub fn new(size: usize) -> Self {
        Self { taffy: Rc::new(RefCell::new(taffy::Taffy::with_capacity(size))) }
    }
}

@gigasource
Copy link

i try to create 10000 nodes from rust side but it has the same result .

@nicoburns
Copy link
Collaborator Author

@gigasource Thanks! Sorry to keep bothering you, but do you also have:

  • Notes on how you compiled and imported Taffy for this test (I haven't actually got this setup myself yet!)
  • A similar script for Yoga (that presumably runs faster)

?

@gigasource
Copy link

gigasource commented Mar 20, 2023

import {Node, Allocator, Position, AlignContent, Layout, FlexDirection} from '@/shared/taffy/taffy_layout';

and i need to setup vite:

const wasm = require("vite-plugin-wasm").default;
\\...
plugins: [
        wasm()
]

@gigasource
Copy link

gigasource commented Mar 20, 2023

for compile i use wasm-pack build --release in your bindings/wasm
and i copy the compiled folder to @/shared/taffy

@gigasource
Copy link

gigasource commented Mar 20, 2023

import initYoga, { FLEX_DIRECTION_ROW } from "yoga-wasm-web";
import wasmUrl from 'yoga-wasm-web/dist/yoga.wasm?url';

const Yoga = await initYoga(
	await fetch(wasmUrl).then(res => res.arrayBuffer())
)

const page = Yoga.Node.create();

console.time('create nodes')
for (let i = 0; i < 10000; i++) {
	const node = Yoga.Node.create();
	page.insertChild(node, i);
}
console.timeEnd('create nodes')

result: create nodes: 26.303955078125 ms

@gigasource
Copy link

i think it takes time for allocate memory because the yoga-wasm-web takes 16MB in head at beginning

@nicoburns
Copy link
Collaborator Author

@gigasource Thanks! I'll look into this properly later, but I think you ought to set the flex_grow and height properties in the yoga benchmark for a fair comparison. Also, you're pushing the nodes to an array in the Taffy benchmark but not in the Yoga one. (it's entirely possible this doesn't make a difference, but worth checking).

You may well be right that it's allocations:

  • Taffy with with_capacity doesn't allocate for the nodes themselves. But it does still allocate a Vec for the children.
  • There is also the style definition objects allocated on the JavaScript side.

@nicoburns
Copy link
Collaborator Author

i try to create 10000 nodes from rust side but it has the same result .

Interesting. We probably ought to test how long it takes when compiling Rust natively!

@gigasource
Copy link

gigasource commented Mar 20, 2023

i try to create 10000 nodes from rust side but it has the same result .

Interesting. We probably ought to test how long it takes when compiling Rust natively!

#[wasm_bindgen]
#[derive(Clone)]
pub struct NodeHub {
}

#[wasm_bindgen]
impl NodeHub {
    #[wasm_bindgen(js_name = createNodes)]
    pub fn createNodes(&mut self, allocator: &Allocator, style: &JsValue) {
        let mut v: Vec<taffy::node::Node> = Vec::new();
        for number in (1..10000).rev() {
            let node = allocator.taffy.borrow_mut().new_leaf(parse_style(&style)).unwrap();
            v.push(node);
        }
    }
}

yes, i try it without style but the same result.

@gigasource
Copy link

https://github.com/facebook/yoga/blob/main/yoga/Yoga.cpp#L159
And can you later implement this function: getHasNewLayout : so i don't need to traverse all the sub-nodes to check.

@gigasource
Copy link

    #[wasm_bindgen(js_name = getLayout)]
    pub fn get_layout(&mut self) -> Layout {
        Layout::new(&self.allocator, self.node)
    }

and i think it is necessary for Node , so i can get directly layout from one node.

@nicoburns
Copy link
Collaborator Author

https://github.com/facebook/yoga/blob/main/yoga/Yoga.cpp#L159 And can you later implement this function: getHasNewLayout : so i don't need to traverse all the sub-nodes to check.

Can you open a separate issue for that? Because that will need to be added to the base library as well as just the WASM bindings. Are the semantics of this method "the layout of this node was updated in the most recent call to compute_layout"?

@nicoburns
Copy link
Collaborator Author

    #[wasm_bindgen(js_name = getLayout)]
    pub fn get_layout(&mut self) -> Layout {
        Layout::new(&self.allocator, self.node)
    }

and i think it is necessary for Node , so i can get directly layout from one node.

This one seems easy enough, although we'll need to check what the performance is like. It may be that the API is designed the way that it is in order to minimise the number of times the JS <-> WASM boundary is crossed (although it may well also be the case that the cost of that has decreased significantly since the API was designed).

@gigasource
Copy link

https://github.com/facebook/yoga/blob/main/yoga/Yoga.cpp#L159 And can you later implement this function: getHasNewLayout : so i don't need to traverse all the sub-nodes to check.

Can you open a separate issue for that? Because that will need to be added to the base library as well as just the WASM bindings. Are the semantics of this method "the layout of this node was updated in the most recent call to compute_layout"?

Yes , correct, but if the result of getHasNewLayout false is, so i don't need to check all the sub-nodes.

@nicoburns
Copy link
Collaborator Author

nicoburns commented Mar 20, 2023

Ok, so the thing that's being slow is the parse_style function. I suspect this is due to repeatedly querying the JsValue accross the JS <-> WASM boundary rather than the actual string parsing being slow. The fact that it does this for every single value regardless of whether it is set is particularly damning.

Modifying the bindings to use Style::DEFAULT instead of parsing the style input brings Taffy down to around the same timing as Yoga (around 5-20ms on my machine). And timings remained similar when I added a couple of yoga-style setter methods to Taffy, and called them on both Yoga and Taffy. Which exactly is faster seems to depend on the browser used, which order the functions are called in, and whether a warmup is done.

With a warmup, timings are more consistent and:

  • In Chrome, Yoga is slightly faster (~15ms vs ~19ms). But there's not much in it.
  • In Firefox, Taffy is mostly about twice as fast (~5ms vs ~10ms).
  • In Safari, Taffy is mostly twice as fast (~5ms vs ~10ms) on the first page load. But then gets much slower (~160ms) if you reload the page, while Yoga stays around the same. Closing the tab and opening it fresh resets this. This makes zero sense to me and is presumably a bug in Safari.

Speaking of a warmup period. I tried using 1000 iterations for the warmup instead of 100 and both Yoga and Taffy crash if tree creation code is repeated 1000 times! Which is not great. I think they're hitting OOM errors, but I'm not sure why because the code should be clearing up after itself.

In any case, I think we will need to abandon these wasm binding's current public API and create an API more like Yoga's which has a seperate setter for each style property. We could also try an API which accepted a single CSS string containing all desired styles and parsed it as CSS. But that's a lot more work, so I think we'll leave that for the time being.

@thecodrr
Copy link

thecodrr commented Jan 18, 2024

@nicoburns The bindings are generating 2 childCount members (one is a method, the other is a field) in the Node class. Is that intentional? TypeScript is giving the following error:

Duplicate identifier 'childCount'.ts(2300)
All declarations of 'childCount' must have identical modifiers.ts(2687)
Subsequent property declarations must have the same type.  Property 'childCount' must be of type '() => number', but here has type 'number'.ts(2717)
taffy_layout.d.ts(351, 3): 'childCount' was also declared here.
(method) Node.childCount(): number

@nicoburns
Copy link
Collaborator Author

@nicoburns The bindings are generating 2 childCount members (one is a method, the other is a field) in the Node class. Is that intentional? TypeScript is giving the following error:

This was very much not intentional. Does the latest commit fix it?

Out of interest, how are you consuming these bindings? (I'm trying to work out how this is likely to be consumed in the real world so that I can make it as easy as possible)

@thecodrr
Copy link

thecodrr commented Jan 18, 2024

This was very much not intentional. Does the latest commit fix it?

Yes, it's fixed. I also noticed an inconsistency between Layout and Node API: new Layout().childCount vs new Node().childCount(). One is a simple getter while the other is a full blown method.

Out of interest, how are you consuming these bindings? (I'm trying to work out how this is likely to be consumed in the real world so that I can make it as easy as possible)

Currently, I do wasm-pack build --release then copy over pkg folder to my own project (renaming the folder to taffy-layout) where I then run npm i ../taffy-layout. After that taffy-layout can be imported as import { Node } from "taffy-layout"

Ideally, once this is published to npm, I'd just do npm i taffy or npm i taffy-layout.

@nicoburns
Copy link
Collaborator Author

Currently, I do wasm-pack build --release then copy over pkg folder to my own project (renaming the folder to taffy-layout) where I then run npm i ../taffy-layout. After that taffy-layout can be imported as import { Node } from "taffy-layout"

Ideally, once this is published to npm, I'd just do npm i taffy or npm i taffy-layout.

Gotcha. One further question: in what context is that where is that import { Node } from "taffy-layout" running? Node.js? A browser? Via a bundler?

I also noticed an inconsistency between Layout and Node API: new Layout().childCount vs new Node().childCount(). One is a simple getter while the other is a full blown method.

Hmm... that represents reality in that on Layout it's a local property whereas on Node it has to do work to fetch the count. But I suppose we could potentially hide that from the user in the name of consistency.

@thecodrr
Copy link

There's also a missing type here for some reason:

computeLayout(size: any): Layout;

What is size? Shouldn't it's type be defined?

@thecodrr
Copy link

Gotcha. One further question: in what context is that where is that import { Node } from "taffy-layout" running? Node.js? A browser? Via a bundler?

Bundler. However, since wasm-pack supports Node.js out of the box, running it directly via a Node.js script shouldn't be an issue.

@nicoburns
Copy link
Collaborator Author

What is size? Shouldn't it's type be defined?

It's an:

interface {
    width: number | "min-content" | "max-content";
    height: number | "min-content" | "max-content";
}

I guess it will be coming through as any as I'm using JsValue on the Rust side. I'll have to look into what the best way of typing this is.

@thecodrr
Copy link

@nicoburns Here are a few things I found lacking after trying out the WASM bindings:

  1. No way to get styles (e.g. getHeight etc.)
  2. The StyleUnit type is incorrectly used in some places which should only accept Dimension
  3. Some function parameters are still typed as any. Maybe we should use typescript_type from wasm_bindgen in those places?
  4. The Layout does not include padding, border or content{Height,Width} values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasm bindings
8 participants