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

Update Binaryen to latest / allowInliningFunctionsWithLoops bindings #1449

Merged
merged 12 commits into from
Sep 11, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Aug 28, 2020

Also adds bindings for BinaryenGetAllowInliningFunctionsWithLoops and BinaryenSetAllowInliningFunctionsWithLoops.

  • I've read the contributing guidelines

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 28, 2020

Tests are failing due to the new APIs not being exported yet.

src/module.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 29, 2020

Interesting, setting allowHeavyweight to true doesn't change any fixtures. Is this expected?

@MaxGraey
Copy link
Member

Interesting, setting allowHeavyweight to true doesn't change any fixtures. Is this expected?

Yes. It's only affected to exported function which relatively small but contain for example loops. See this test

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 29, 2020

Can you add a test for this in a follow-up PR?

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 29, 2020

Also, I don't see how this now affects optimize levels, for instance whether it can theoretically yield larger modules on high shrink levels, and I'd appreciate if you could explain :)

@MaxGraey
Copy link
Member

MaxGraey commented Aug 29, 2020

Also, I don't see how this now affects optimize levels, for instance whether it can theoretically yield larger modules on high shrink levels, and I'd appreciate if you could explain :)

No It's affect only to -O3 on binaryen side. It's hardcoded as:

return options.optimizeLevel >= 3 && options.shrinkLevel == 0 &&
           (lightweight || options.inlining.allowHeavyweight);

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 29, 2020

What do you think of setting this option to true only if optimizeLevel > 2 on our end then, just to be sure? Would be clearer to me.

@MaxGraey
Copy link
Member

What do you think of setting this option to true only if optimizeLevel > 2 on our end then, just to be sure? Would be clearer to me.

ok

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 29, 2020

Let me know if this does what you want now :)

@MaxGraey
Copy link
Member

MaxGraey commented Aug 29, 2020

LGTM. Thanks!

src/module.ts Outdated Show resolved Hide resolved
src/glue/binaryen.d.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
dcodeIO and others added 5 commits September 6, 2020 19:12
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

Interesting, hitting an assertion now indicating that somewhere somehow an expression is null.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 6, 2020

Interesting, hitting an assertion now indicating that somewhere somehow an expression is null.

Yes, and it's happening in wasm-traversal.h in this line. Could you check previous version of binaryen which contain this PR?

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

Full error isn't super useful unfortunately

Assertion failed: *task.currp, at: /home/runner/work/binaryen.js/binaryen.js/binaryen/src/wasm-traversal.h,702,walk
Error: abort(Assertion failed: *task.currp, at: /home/runner/work/binaryen.js/binaryen.js/binaryen/src/wasm-traversal.h,702,walk). Build with -s ASSERTIONS=1 for more info.
    at q (\path\to\assemblyscript\node_modules\binaryen\index.js:55:227)
    at i (\path\to\assemblyscript\node_modules\binaryen\index.js:104:50)
    at ACb (\path\to\assemblyscript\node_modules\binaryen\index.js:17:573541)
    at Pc (\path\to\assemblyscript\node_modules\binaryen\index.js:177:266)
    at w8 (\path\to\assemblyscript\node_modules\binaryen\index.js:17:686915)
    at CCb (\path\to\assemblyscript\node_modules\binaryen\index.js:13:2243554)
    at maa (\path\to\assemblyscript\node_modules\binaryen\index.js:13:1201277)
    at rJb (\path\to\assemblyscript\node_modules\binaryen\index.js:25:188687)
    at CJb (\path\to\assemblyscript\node_modules\binaryen\index.js:25:361239)
    at Oc (\path\to\assemblyscript\node_modules\binaryen\index.js:177:367)
    at EJb (\path\to\assemblyscript\node_modules\binaryen\index.js:13:1593565)
    at pLb (\path\to\assemblyscript\node_modules\binaryen\index.js:25:783)
    at GLb (\path\to\assemblyscript\node_modules\binaryen\index.js:25:2271)
    at jfb (\path\to\assemblyscript\node_modules\binaryen\index.js:17:392657)
    at Oc (\path\to\assemblyscript\node_modules\binaryen\index.js:177:367)
    at Object.pgb [as _BinaryenModuleAllocateAndWrite] (\path\to\assemblyscript\node_modules\binaryen\index.js:13:2136119)
    at Module.toBinary (\path\to\assemblyscript\src\module.ts:1695:14)
    at \path\to\assemblyscript\cli\asc.js:830:23
    at measure (\path\to\assemblyscript\cli\asc.js:1103:3)
    at Object.main (\path\to\assemblyscript\cli\asc.js:829:25)
    at \path\to\assemblyscript\bin\asc:21:47

@MaxGraey
Copy link
Member

MaxGraey commented Sep 6, 2020

also could you disable setAllowInliningFunctionsWithLoops(false) for exclude issue with inlining?

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

Error also happens if I comment out the setAllowInliningFunctionsWithLoops lines. Going to look for the last working nightly.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

Last working is 96.0.0-nightly.20200903.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

@MaxGraey
Copy link
Member

MaxGraey commented Sep 6, 2020

I think it's last one. Could you open issue?

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 6, 2020

Perhaps, before opening an issue, we might try to see if we can find the problem by injecting a couple checks for null in Binaryen itself. I guess a PR just fixing this would be ideal :)

@MaxGraey
Copy link
Member

MaxGraey commented Sep 6, 2020

I'm not sure this a good idea. It will be much harder try to fix from our side

@dcodeIO dcodeIO changed the title Update Binaryen to latest / allowHeavyweight bindings Update Binaryen to latest / allowInliningFunctionsWithLoops bindings Sep 11, 2020
@dcodeIO dcodeIO merged commit c76eb81 into master Sep 11, 2020
@github-actions
Copy link

🎉 This PR is included in version 0.14.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MaxGraey MaxGraey deleted the allowHeavyweight branch November 29, 2020 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants