Adds Javascript Support #304

arcanis · 2016-12-26T10:58:46Z

As mentioned in the title, this PR adds Javascript support to Yoga. Two different builds are included in this PR thanks to nbind, which conveniently allow to target both Node.js' native addons and browser environments via asmjs with approximately the same codebase. That should solve JavaScript variant planned? #215.
All tests successfully pass on both codepaths. You can run yarn test:all inside the javascript directory to test it.
Because of a bug in nbind, the following PR needs to be merged and a new version released before this one can be safely merged as well.
I had to use double types instead of float in the C++ bindings because of an Emscripten bug where symbols aren't correctly exported when using floats.
There's some tweaks to do before this PR is 100% ready to merge, but I wanted to have your opinion first. What do you think of this?

To do:

emilsjolander · 2016-12-26T11:02:56Z

#299 Also adds javascript support via emscrimten and is nearly ready to be merged. Could you collaborate with @mikearmstrong001 on adding anything that might be missing from that PR?

arcanis · 2016-12-26T11:23:33Z

Aww I completely missed it, sorry @mikearmstrong001 :(

Looking at it, I'm not sure anything is missing per-se, but there's two major differences between our PRs:

WIP: JavaScript port via emscripten #299 doesn't export a native Node.js module. I think it's the main difference, and maybe the most difficult one to implement since it requires either a lot of glue code, or a dedicated build chain. That's why I chose nbind, which nicely abstract the interfacing of both Node.js addons and asmjs modules under the same umbrella.
This PR uses an OOP API which might be a bit more idiomatic than free functions, at least in Javascript. That's kind of a philosophical choice, so there's really nothing to "fix" on WIP: JavaScript port via emscripten #299.

"Fixing" both of these issues to match the behaviour in this PR would probably result in a PR very similar to mine, so I guess you'll have to choose which one of those two approaches you prefer.

emilsjolander · 2016-12-26T11:32:15Z

#299 doesn't export a native Node.js module. I think it's the main difference, and maybe the most difficult one to implement since it requires either a lot of glue code, or a dedicated build chain. That's why I chose nbind, which nicely abstract the interfacing of both Node.js addons and asmjs modules under the same umbrella.

Sounds cool, I don't know anything about this. @mattpodwysocki was looking at building node.js bindings. Wonder if he has any comments.

This PR uses an OOP API which might be a bit more idiomatic than free functions, at least in Javascript. That's kind of a philosophical choice, so there's really nothing to "fix" on #299.

If you read the comments in the other pull request you can see that this is the way we were going with it 👍

"Fixing" both of these issues to match the behaviour in mine would probably end up in a PR very similar to mine, so I guess you'll have to choose which one of those two approaches you prefer.

Agreed, we will most likely end up just merging one of these. Would love some input from people who know more than me about this. cc: @mikearmstrong001 @vjeux

vjeux · 2016-12-26T11:57:36Z

I'm not really familiar with the c -> js options.

The only thing I know is that it is very annoying when a node dependency has a native module as any time you npm install it it takes forever to actually build it. But, on the other hand, it's probably unfortunate to lose the performance when you can actually run C code.

Anyway, I don't have much to add here.

emilsjolander · 2016-12-26T12:10:45Z

@arcanis I'll await comments @mikearmstrong001 on using nbind before conducting a code review.

mikearmstrong001 · 2016-12-26T12:41:03Z

As @emilsjolander and I discussed: I actually have moved to and arrived at the same interface through a different process so the lack of a native implementation is the difference.

We should go with the solution that solves the most problems with the minimum effort! which seems to be this PR, as long as it works for day to day maintenance, handles running on a browser as is not ill performant I'm happy.

I'll check my requirements and get back very soon. Nice work @arcanis I've not used or looked at nbind before.

emilsjolander · 2016-12-26T12:43:38Z

@mikearmstrong001 awesome! Would be super helpful if you can help check that this meets all the requirements you have.

@arcanis Can you start by making this PR run the JS tests in travis?

arcanis · 2016-12-26T12:47:58Z

@mikearmstrong001 great ! thanks !

@emilsjolander sure - native tests should be easy enough to run, asmjs not that much (the main issue I had when I tried this task previously is that building the asmjs version requires emscripten, which in turn requires to build llvm, which is too heavy for Travis CI). Would that work for you?

I'm currently working on implementing measureFunc and it should be mostly done. However, it seems there is no automatic measurement test for non-cpp languages, is that correct?

emilsjolander · 2016-12-26T12:53:00Z

@arcanis I would like us to run the javascript tests in CI. In #299 the output of emscripten was checked in. Can you do similar here? Or can we pre-build an emscripten binary for travis?

I'm currently working on implementing measureFunc and it should be mostly done. However, it seems there is no automatic measurement test for non-cpp languages, is that correct?

Not sure what you mean, could you explain?

mikearmstrong001 · 2016-12-26T12:53:24Z

measureFun: @arcanis that is correct, a reasonable base to work with is the Benchmark or a variant of the cpp YGMeasureTest

arcanis · 2016-12-26T12:58:35Z

@arcanis I would like us to run the javascript tests in CI. In #299 the output of emscripten was checked in. Can you do similar here? Or can we pre-build an emscripten binary for travis?

Hm, I think you're right, commiting the pre-built asmjs file to the repository might be good enough. Only issue I see is that it will not correctly detect modifications if a contributor forget to rebuild it before pushing.

@mikearmstrong001 Good idea, I'll use your benchmark !

emilsjolander · 2016-12-26T13:00:30Z

@arcanis Yeah that is the downside. Can you look into getting a pre-build emscripten binary running on travis. That would be awesome.

mikearmstrong001 · 2016-12-26T13:01:45Z

@arcanis how are you handling the free functions? they seem to be missing from the generated tests, we should confirm these work as expected. My use case will certainly be hitting these.

I'd also recommend YogaNodeTest in c# this is the final test I was working through. I can help if you could send me something I could run as asm.js

arcanis · 2016-12-26T13:06:36Z

@mikearmstrong001 Which functions do you specifically need? I've currently exported setExperimentalFeatureEnabled and isExperimentalFeatureEnabled (here), and exporting the others shouldn't take too long.

mikearmstrong001 · 2016-12-26T13:42:46Z

The reason I was asking about free is that I need to ensure the the YGNodeRef m_node is correctly deallocated. I'm not familiar on how nbind will handle destructors within JS based code.

Having something in the PR I can run in a browser would be very helpful.

These will need to be exposed and there may be others I've missed.

WIN_EXPORT void YGNodeFree(const YGNodeRef node);
WIN_EXPORT void YGNodeFreeRecursive(const YGNodeRef node);
WIN_EXPORT void YGNodeReset(const YGNodeRef node);
WIN_EXPORT void YGNodeMarkDirty(const YGNodeRef node);
WIN_EXPORT bool YGNodeIsDirty(const YGNodeRef node);
WIN_EXPORT bool YGNodeCanUseCachedMeasurement(const YGMeasureMode widthMode,
WIN_EXPORT void YGNodeCopyStyle(const YGNodeRef dstNode, const YGNodeRef srcNode);
YG_NODE_STYLE_PROPERTY(float, AspectRatio, aspectRatio);
WIN_EXPORT void YGNodeStyleSetFlex(const YGNodeRef node, const float flex);
YG_NODE_PROPERTY(bool, HasNewLayout, hasNewLayout);

By looking at the handwritten test functions eg YGStyleTest these would be actively hit.

arcanis · 2016-12-26T13:53:46Z

I'll check this. According to charto/nbind#34, destructors should be automatically called by the garbage collector in native modules, and by calling the node.free() function inside browsers.

Having something in the PR I can run in a browser would be very helpful.

If you have emcc in your PATH, you should be able to run yarn build:asmjs and use the module with bundlers such as webpack (you can use a symlink from the node_modules/yoga-layout to the javascript directory to make it easier).

mikearmstrong001 · 2016-12-26T14:57:46Z

@emilsjolander and @arcanis for easy of adoption I think prebuilt JS library should be included. Building a JS solution via emcc is going to be a chore for anyone just looking to use the lib.

Can we run tests on a travis generated build and also against the prebuilt solution which we should actively encourage to be unloaded as part of a PR. I'm sure we could look at automating the generation.

arcanis · 2016-12-26T15:02:03Z

The option I went for with https://github.com/manaflair/text-layout was to ship the asmjs file but only on npm, not github, simply by adding a .npmignore with different rules than the .gitignore (the asmjs file is ignored by git, but not by npm), so that emscripten is only required when running yarn publish.

… one

arcanis · 2016-12-30T10:25:01Z

@emilsjolander @mikearmstrong001 I've added a npm run bench script that gives some insight about the perfs of the node module vs the asmjs one, renamed a few other scripts to make everything consistent, rebased the branch on top of master, and added some documentation.

I haven't changed the gist links yet, since I wasn't sure if you wanted to do it yourself or not (also, have you considered using something like gist-it to directly print the relevant source code in the documentation? that would avoid the need to manually update the gist when something changes).

I think this PR is close to be ready, do you see something else? 😊

emilsjolander · 2016-12-30T10:47:41Z

@arcanis This looks good to go from my end. @mikearmstrong001 what do you think?

Regarding using gists, I did it mostly because I like the styling of them and did not want to spend time replicating it 👍 I would rather have the code in the docs file :) I can update them and update the main landing page as well once this is landed.

mikearmstrong001 · 2016-12-30T10:57:51Z

@arcanis Thanks for the doc entries! I wasn't expecting you to take that on as well, really appreciated. I too would like to pull docs straight from code, it works well for React VR :)

I think your approach and justification for Node is good and agree that the error message is somewhat obtuse at
Browser:

Yoga.bundle.js:269 Uncaught TypeError: Cannot read property 'apply' of undefined
    at new Bound (Yoga.bundle.js:269)
   ...

Node

Error: Wrong number of arguments
    at evalmachine.<anonymous>:15:14
    at YGBENCHMARK (/Users/mikea/git/yoga-arc/javascript/tests/run-bench.js:29:21)

But given docs on how to create nodes already exist, this will be fine.

I made changes to your bench test and those are

allow the JS to warm up
run the function several times and average the result

We should apply these to make measurements more valid

Before Change:

Stack with flex
  - node: 23ms (100%)
  - browser: 181ms (786.96%)

Align stretch in undefined axis
  - node: 50ms (100%)
  - browser: 193ms (386%)

Nested flex
  - node: 53ms (100%)
  - browser: 77ms (145.28%)

Huge nested layout
  - node: 18ms (100%)
  - browser: 86ms (477.78%)

After change:

Stack with flex
  - node: 19.4ms (100%)
  - browser: 33.8ms (174.23%)

Align stretch in undefined axis
  - node: 47.4ms (145.4%)
  - browser: 32.6ms (100%)

Nested flex
  - node: 38ms (125.41%)
  - browser: 30.3ms (100%)

Huge nested layout
  - node: 15ms (100%)
  - browser: 27.5ms (183.33%)

                let testEntry = testResults.get(name);

                if (testEntry === undefined)
                    testResults.set(name, testEntry = new Map());

                let WARMUP_IT = 3;
                for (let i=0; i<WARMUP_IT; i++) {
                    fn();
                }

                let BENCHMARK_IT = 10;
                let start = Date.now();
                for (let i=0; i<BENCHMARK_IT; i++) {
                    fn();
                }
                let end = Date.now();

                testEntry.set(type, (end - start) / BENCHMARK_IT);

Great stuff!

emilsjolander · 2016-12-30T11:03:18Z

Thanks! This is an awesome addition. I'll import the PR now and start the merge process

facebook-github-bot · 2016-12-30T11:03:49Z

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mikearmstrong001 · 2016-12-30T11:04:38Z

javascript/sources/entry-common.js

+
+    }
+
+    patch(lib.Node.prototype, `free`, function () {


For future maintainers: perhaps some comments on why these functions are patched.

Just added some comments

arcanis · 2016-12-30T11:04:46Z

Oh right ! Just pushed your changes. Funny how the asmjs build is noticeably faster than the native one on some tests ... maybe some optim flags will need to be tweaked.

emilsjolander · 2016-12-30T11:08:33Z

@arcanis Just noticed that the added files are missing our open source license header. Could you please make sure all the added files have it?

mikearmstrong001 · 2016-12-30T11:11:19Z

Funny how the asmjs build is noticeably faster than the native one on some tests ... maybe some optim flags will need to be tweaked.

Yeah it is interesting, although we definitely want to make sure we are not measuring the JIT :)

I'll run a test against a simple emscripten compile of the code so we can get an understanding of the nbind overhead.

@emilsjolander please note that the benchmark numbers have been changed from the original c version so they shouldn't be compared directly.

@arcanis Again, thanks for your work!

arcanis · 2016-12-30T11:19:03Z

@emilsjolander Yup, here they are !

facebook-github-bot · 2016-12-31T11:21:49Z

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Dec 26, 2016

arcanis force-pushed the emscripten branch from 9ee920a to b0910c0 Compare December 26, 2016 13:40

arcanis force-pushed the emscripten branch from b0910c0 to 80b9856 Compare December 26, 2016 13:50

arcanis force-pushed the emscripten branch 4 times, most recently from 1da7d81 to 7664635 Compare December 26, 2016 14:41

arcanis force-pushed the emscripten branch 5 times, most recently from df4e70f to 78a1e6f Compare December 26, 2016 16:55

Use an actual nbind release instead of a fork

54be57c

arcanis force-pushed the emscripten branch from d5ea6ad to 492e931 Compare December 30, 2016 09:59

arcanis added 10 commits December 30, 2016 11:05

Refactor the scripts

5d35690

Fix issues regarding the garbage collection of currently used pointers

3ea9bc8

Improve resource destruction

23136b9

Add Node.prototype.release

9f78c7e

Force nbind to duplicate its shared pointers rather than use a single…

acac0c6

… one

Remove smart pointers

49e8db7

Add hgignore

9a9f3f1

Add the asmjs file to the versioning system

f8d2749

Add the bench script

13efe4a

Update the js build

f200240

arcanis force-pushed the emscripten branch from 492e931 to f200240 Compare December 30, 2016 10:09

Add javascript to the documentation

dc0229e

arcanis added 2 commits December 30, 2016 11:59

Use gist-it to embed parts of the source code in the documentation

17f217b

Add a warmup phase to the benchmark process

b651297

mikearmstrong001 reviewed Dec 30, 2016

View reviewed changes

Add some comments to the source code

8caecc2

Add facebook copyright banners

51380b2

facebook-github-bot closed this in 6f462a7 Jan 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Javascript Support #304

Adds Javascript Support #304

arcanis commented Dec 26, 2016 •

edited

Loading

emilsjolander commented Dec 26, 2016

arcanis commented Dec 26, 2016 •

edited

Loading

emilsjolander commented Dec 26, 2016

vjeux commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

arcanis commented Dec 26, 2016 •

edited

Loading

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016

arcanis commented Dec 26, 2016 •

edited

Loading

mikearmstrong001 commented Dec 26, 2016

arcanis commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 30, 2016 •

edited

Loading

emilsjolander commented Dec 30, 2016

mikearmstrong001 commented Dec 30, 2016

emilsjolander commented Dec 30, 2016

facebook-github-bot commented Dec 30, 2016

mikearmstrong001 Dec 30, 2016

arcanis Dec 30, 2016

arcanis commented Dec 30, 2016

emilsjolander commented Dec 30, 2016

mikearmstrong001 commented Dec 30, 2016 •

edited

Loading

arcanis commented Dec 30, 2016

facebook-github-bot commented Dec 31, 2016

Adds Javascript Support #304

Adds Javascript Support #304

Conversation

arcanis commented Dec 26, 2016 • edited Loading

emilsjolander commented Dec 26, 2016

arcanis commented Dec 26, 2016 • edited Loading

emilsjolander commented Dec 26, 2016

vjeux commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

arcanis commented Dec 26, 2016 • edited Loading

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016 • edited Loading

arcanis commented Dec 26, 2016

emilsjolander commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016 • edited Loading

arcanis commented Dec 26, 2016

mikearmstrong001 commented Dec 26, 2016

arcanis commented Dec 26, 2016 • edited Loading

mikearmstrong001 commented Dec 26, 2016

arcanis commented Dec 26, 2016 • edited Loading

arcanis commented Dec 30, 2016 • edited Loading

emilsjolander commented Dec 30, 2016

mikearmstrong001 commented Dec 30, 2016

emilsjolander commented Dec 30, 2016

facebook-github-bot commented Dec 30, 2016

mikearmstrong001 Dec 30, 2016

Choose a reason for hiding this comment

arcanis Dec 30, 2016

Choose a reason for hiding this comment

arcanis commented Dec 30, 2016

emilsjolander commented Dec 30, 2016

mikearmstrong001 commented Dec 30, 2016 • edited Loading

arcanis commented Dec 30, 2016

facebook-github-bot commented Dec 31, 2016

arcanis commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016 •

edited

Loading

mikearmstrong001 commented Dec 26, 2016 •

edited

Loading

mikearmstrong001 commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 26, 2016 •

edited

Loading

arcanis commented Dec 30, 2016 •

edited

Loading

mikearmstrong001 commented Dec 30, 2016 •

edited

Loading