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

Use Binaryen's directed inlining #460

Closed
wants to merge 6 commits into from
Closed

Use Binaryen's directed inlining #460

wants to merge 6 commits into from

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Feb 5, 2019

This replaces our custom inlining with what's proposed in this Binaryen PR. Uses a custom Binaryen build for that purpose currently and probably needs some additional work, but pinning it here already so we can have a look how that'd turn out.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

Also need fix warnings like WARNING AS206: Compiling constant with non-constant initializer as mutable. somehow

@dcodeIO
Copy link
Member Author

dcodeIO commented Feb 5, 2019

Hmm, that might turn out to be quite a problem. Previously, we could just precompute inlined calls, which we can't anymore because inlining is performed on finalization. That's actually a good reason to keep our own inlining, unless we could BinaryenCallInline directly as noted in the other PR, which would require some significant changes to how Binaryen's code generation works... :(

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

binaryen has method which could check cost for function and get possibility for inlining it and also side effect checking. So what if just check if it return true compiler try evaluate expression/function and try retrieve constant as result?

@dcodeIO
Copy link
Member Author

dcodeIO commented Feb 5, 2019

Yeah, good idea. Precompute would essentially have to evaluate called function bodies for this. Not just constant returns, though, but also calls to other functions that might again be inlined etc. (probably everything)

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

Hmm. It seems make sense keep some own inline stuff specially for class field's initialization

@dcodeIO
Copy link
Member Author

dcodeIO commented Feb 5, 2019

Plus, seems the build breaks because of webpack running out of memory (or something, again). I didn't see that coming.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

Try to update webpack to latest. It may be this already fixed bug: webpack/webpack#8639

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

It seems same issue:
webpack/webpack#6389

@MaxGraey
Copy link
Member

MaxGraey commented Feb 5, 2019

Need to try node --max_old_space_size=2048 ./node_modules/webpack/ ... or try disable source maps for uglify plug-in

cc @xtuc

@dcodeIO dcodeIO mentioned this pull request Feb 6, 2019
@dcodeIO
Copy link
Member Author

dcodeIO commented Feb 6, 2019

Closing in favor of #463

@dcodeIO dcodeIO closed this Feb 6, 2019
@dcodeIO dcodeIO deleted the directed-inlining branch March 7, 2019 23:50
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.

None yet

2 participants