Migrate to emscripten #1177
Assignees
DaddyFrosty
Clear assignees
DaddyFrosty
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: DaddyFrosty/yoga#1177
Reference in New Issue
Block a user
No description provided.
Delete Branch "emscripten"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I left these as
YGValuesin the hope that in the future, we add support for percentages. I just couldn't figure it out at the time@NickGerleman hello, I'm back
The bindings are ready so let's discuss the migration and the next steps.
I just replaced
nbindwithembindand removed allnbindcode. Is it okay or it would be better to maintain both versions while migrating?I wanna provide separate
dev(with some additional assertions and checks) andprod(minified and with optimizations) bundles, so I need some bundler (rollup). What do you think about that? maybe you have suggestions here.I don't know what to do with CI. Can I setup GitHub action for testing like this or it is not a big deal rn?
this makes sense now, thanks for the clarification
but I have problems with types
YGNodeStyleGetGapreturnsfloatthat can't be converted withValue::fromYGValueI think that is better to fix
YGNodeStyleGetGapfirst, it should returnYGValueI can't pick the unit for this type now (should it be undefined or auto or digit?)
Ah ok - didn't realise the these functions were inconsistent. The unit is
YGUnitPointThis is incredible! I will take a close look soon.
I think it is okay to fully replace nbind. It has been a long time since folks used it for Node native module versions of Yoga compared to packages like yoga-layout-prebuilt, so it is better imo to have something working, compared to full parity.
Re dev vs prod, I know Emscripten has options for what it emits (it will already minify), so I’m guessing the extra bits would be for the JavaScript frontend to it? If we can avoid the complexity, it might be better to distribute the package without running through another bundler, then the consuming project could bundle using the bundler they are already using.
I’d love it if we could add GitHub Actions validation, but I know it isn’t present right now. There are some workflows I added recently that could be pattern matched, and it would document how we are running tests against this. The website build is currently using the last published version of yoga-layout instead of the one in the repo too, so we don’t get validation from the website build job (tried to move this to workspaces recently but the website setup needs some serious renovation to support it).
I got the warning
do you have any idea why it is here and maybe it can be fixed
I think this means the build isn’t picking up yoga/event/event.cpp.
@jacobp100 I added in support for percentage gaps when I ported your gap changes to Taffy (a rust flexbox library originally based off of Yoga). Diff is here if it helps you figure things out. There are also test fixtures for percentage based gaps in that PR (same format as yoga, except we have each fixture in a separate file).
I'm super grateful for this work. Given how the nbind version was long-term broken, we should be aggressive in taking improvements here.
If you don't mind, I'm going to try to push a commit to this PR adding the
yarn testscript you wrote to GitHub workflows. The previous lack of coverage for bindings let a lot of bad code slip into them, so if we have a working test script, it would help to both protect the change and demonstrate it is working when I import it 🙂.Reading, this is smaller than the default dlmalloc, but may perform worse. Do you know if it has a performance impact for Yoga?
We should add a
-std=c++xxhere somewhere (was previously 14, but other bits build as 11)This will effectively break C++ exceptions, so we should also probably go ahead and add
-fno-exceptions(and-fno-rttiwould probably add some more savings)One more, we should consider adding
-fltowhich could make things a bit smaller/faster.That seems like a great help 🙂.
During H1 2023 I have some work on my plate to try to port some of the layout examples in WPT to Yoga compatible fixtures as well. Seems like a great resource for validating correctness of new layout props outside the context of browsers.
Ah, thanks for putting me onto Web Platform Tests I knew there must be something like this, but I hadn't been able to find it. I'm about to create a test suite for a CSS Grid implementation, and that looks like a great source of test cases, or at least inspiration.
Testing locally:
emmalloc: 165kb
dlmalloc: 173kb
Disabling exceptions and RTTI saves 10kb. LTCG does not lead to size savings, but is best practice for performance so I am tempted to think we should enable it (despite impact to build perf of the Yoga repo).
Yes, I tested it too and it doesn't affect the perf
yep, sure
emccbuild takes 2 times longer but it definitely makes sensethanks for suggestion
🚀
I pushed a couple of changes here:
Here is the performance difference between asm.js and wasm being run by Node from the scenarios in the run-bench script.
Interesting. I'm not sure how representative these benchmarks are, but ~13ms for the "huge nested layout" was also what I was getting by using the
yoga-layout-prebuiltpackage from NPM (although I was only measuring layout computation time and not including the tree setup time there).I can't understand why "Align stretch in undefined axis" is so slow though. That only has 10 nodes (compared to 10,000 for the huge nested layout)! Admittedly it's not the same benchmark, but I was able to run a variant of the "huge nested layout" with depth of 1 instead of depth of 4 (so 10^1 = 10 rather than 10^4 = 10,000 nodes) in ~45 micro seconds.
One thing I did notice when benchmarking
yoga-layout-prebuiltwas that there seemed to be a fixed ~30ms penalty the first time I calledcalculate_layout()(can't remember exact name in yoga) in a given run (this happened even if other functions to setup nodes had already been called. So in order to get accurate results I had to run the benchmark once and throw away the result before collecting measurements. I wonder if a similar penalty might be skewing the results here. I would say that the results of the 4 benchmarks are suspiciously similar.I haven't tried to debug it to see if it is working as expected, but there is some mention in the script on skipping the first few iterations to avoid one-time costs, then averaging the rest.
Some of the tests I can see like "Align stretch in undefined axis" add 2000 child nodes under the root node, so there still is a lot there. That 2000 node count is the same for other tests, but "huge nested layout" is triply nested of 6 children it looks like.
Ah, I can what I've done here. I'm reading the C benchmarks and assuming the JavaScript benchmarks are the same! The C benchmarks with the same name is only using 10 child nodes. These numbers make a lot more sense at a count of 2000 nodes.
Looking at the detail of Chrome not allowing synchronous compilation of WebAssembly files> 4KB, we should bite the bullet and change the bindings to require async initialization I think.
I like async initialization on default, but it is better to have a sync version too (for asm) Because it is a pain to migrate
Yogafrom sync to async version on platforms that don't support top-level awaitI think it would be reasonable to have a
yoga-layout/syncdocumented in the readme which points to a synchronously obtained asmjs build. Technically, we could go further and have both sync and async asmjs and wasm builds, since Node doesn't have the same sync compilation limitation that Chrome does. So we could have a default async variant, then the sync one would be the same export map as the async one, minus browsers getting the asmjs version.@jeetiss What do you think of a design where we:
await Yoga.load()or similar to allow for async wasm compilation. Default to wasm for node, browser, but default to asm elsewhere or for old clients that don't understand export maps./syncentrypoint provides the legacy sync API, but falls back to asm.js on browsers to allow it (at a size/performance cost), but we still let Node use sync wasm compilationThis change in general is a breaking API change compared to the last package, and means semver wise we ideally ought to do a
yoga@2.0.0(hoping it doesn't create confusion as Yoga 2 is a bit overloaded lol). I still don't have a publishing pipeline set up, and might end up just starting with Maven/npm since the Apple/YogaKit build is not in a great state and I haven't built the context to really critically understand the set of problems and fixes for it.After this change, but before we publish that new breaking version, it might make sense to make the change to
YGNodeStyleGetGapto move toYGValue(@jacobp100).Also want to apologize for commandeering this PR a bit 😅. Original goal was just to add GitHub actions/apply the flag changes, but I got a little carried away hacking at this and trying to understand everything here.
@NickGerleman sounds awesome, I am glad that you are pushing this forward 🚀
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@NickGerleman merged this pull request in facebook/yoga@1813748eaa.
Pull request closed