Add optimize, shrink level and debug info options to C/JS #1357

dcodeIO · 2018-01-11T03:09:16Z

Adds the following functions to the C-API:

BinaryenGetOptimizeLevel()
BinaryenSetOptimizeLevel(int level)
BinaryenGetShrinkLevel()
BinaryenSetShrinkLevel(int level)
BinaryenGetDebugInfo()
BinaryenSetDebugInfo(int on)

Respectively the following to the JS-API:

Binaryen.getOptimizeLevel()
Binaryen.setOptimizeLevel(level)
Binaryen.getShrinkLevel()
Binaryen.setShrinkLevel(level)
Binaryen.getDebugInfo()
Binaryen.setDebugInfo(on)

debugInfo is currently used as an option on the binary writer only and has no effect on text format.

Default optimizeLevel is 2 and default shrinkLevel is 1, which equals -O / -Os. If I am not mistaken, binaryen.js previously always used other levels (something like default passes with 0 / 0), which would explain this select issue.

Also reverts the MODULARIZE change to binaryen.js in an attempt to make it work again, for now, until MODULARIZE_INSTANCE or similar has found its way into Emscripten. This now differes a bit from how it was before in that it tries to resemble MODULARIZE as much as possible (binaryen.js-pre.js is just the MODULARIZE header), but exports a singleton instance on top of it using this recently proposed webpack loader code.

Also provides Binaryen.instantiate in JS to create a unique new instance of the API with its own memory etc.

…s of the API

Tests misuse a module as a script by concatenating, so instead of catching this case in the library, catch it there

Seems optimized output changed due to running with optimize levels 2/1 now

dcodeIO · 2018-01-11T17:41:04Z

Appears the JS tests are working now, but there's a leak sanitizer error somewhere else. What have I done.

…vels

kripken · 2018-01-11T19:36:14Z

The leak sanitizer can be ignored, it's unrelated breakage. I need to land something to disable it.

kripken

Let's also add the testcase with the if not turning into a select, it will catch us setting the shrink level properly by default.

kripken · 2018-01-11T19:37:49Z

build-js.sh

@@ -579,10 +583,5 @@ export_function "_BinaryenSetAPITracing"
  -Isrc/ \
  -s EXPORTED_FUNCTIONS=[${EXPORTED_FUNCTIONS}] \
  -o bin/binaryen${OUT_FILE_SUFFIX}.js \
-  --post-js src/js/binaryen.js-post.js \


have we decided that MODULARIZE_INSTANCE is the way to go here?

I'm not objecting, just I thought it was still being discussed, but I could be wrong.

In any case, is it worth undoing this, this re-doing it all with MODULARIZE_INSTANCE?

Possibly not when you are asking. Though, previously, the global var was a singleton instance (through concatenation of Binaryen = Binaryen()) while module loaders received a factory function. My intention here was to make sure that module loaders receive the singleton instance as well so it's consistent and backwards-compatible until something else is in place.

Yeah, consistency is important, I agree. But I can open a PR for MODULARIZE_INSTANCE later today, so it shouldn't be long before we can use it. If you don't mind undoing the changes here after that, though, then it doesn't matter either way, I just thought it might be efficient to wait.

Sure, is fine for me either way.

kripken · 2018-01-11T19:38:53Z

check.py

@@ -360,6 +360,10 @@ def run_binaryen_js_tests():
    f = open('a.js', 'w')
    binaryen_js = open(os.path.join(options.binaryen_bin, 'binaryen.js')).read()
    f.write(binaryen_js)
+    # running concatenated versions in node interferes with module loading because


there is similar code in auto_update_tests.py.

we need to verify that code works in both spidermonkey and node.js. the bots test just one.

Assumption here is that, in spidermonkey, there isn't a global module or exports so that the last case of the module loader sets up a global, just like in a browser

else (typeof self !== "undefined" ? self : this)['Binaryen'] = instantiate();

and the workaround is skipped (condition is false).

if (typeof module === "object" && typeof exports === "object") Binaryen = module.exports;

But I agree, that should also be tested.

kripken · 2018-01-11T19:40:26Z

src/binaryen-c.cpp

@@ -70,6 +70,11 @@ Literal fromBinaryenLiteral(BinaryenLiteral x) {
 static std::mutex BinaryenFunctionMutex;
 static std::mutex BinaryenFunctionTypeMutex;

+// Optimization options (default is -Os with debug info)
+static int optimizeLevel = 2;


maybe there's a way to reuse src/tools/optimization-options.h which has those constants? or to share code with it.

That was my first thought as well but then I discovered that tools/optimization-options.h has a dependency on Options in support/command-line.h which felt wrong to include.

True. We should probably do a general refactoring of those defaults, probably into pass.h. I can do that unless you want to.

Currently the defaults are in support/defaults.h, which is then included in binaryen-c.h and tools/optimization-options.h. I'd probably just move them then, so it might be better when you'll take a look.

kripken · 2018-01-11T19:40:42Z

src/binaryen-c.cpp

+// Optimization options (default is -Os with debug info)
+static int optimizeLevel = 2;
+static int shrinkLevel = 1;
+static bool debugInfo = 1;


debug info should be off by default, i think.

kripken · 2018-01-11T19:42:36Z

src/binaryen-c.h

@@ -659,6 +659,15 @@ int BinaryenModuleValidate(BinaryenModuleRef module);
 // Runs the standard optimization passes on the module.
 void BinaryenModuleOptimize(BinaryenModuleRef module);

+// Sets the optimization level to use. 0, 1, 2 correspond to -O0, -O1, -O2, etc.
+void BinaryenSetOptimizeLevel(int level);


Alternatively to this API, another option is to just expect people to use the existing RunPasses on inputs like -O etc. - same as on the console? That could trivially reuse the code from src/tools/optimization-options.h.

Or, there could be an Optimize() variant with more params.

Thoughts?

RunPasses has the disadvantage that one has to know exactly which passes are available in the current version of the API. It also seemed to me that some passes do different things depending on the actual levels?

In this case, all of Optimize, RunPasses, OptimizeFunction and RunPassesOnFunction would need such a variant with more params.

Yeah, good point about passes depending on the optimize levels too. So I think your API choices in this PR make sense.

dcodeIO · 2018-01-11T22:19:44Z

Tests seem to fail now because, with debugInfo being 0 by default, some (or all) wasts don't have names anymore. Previously the API behaved as if debugInfo had been explicitly activated, but I don't know how. Maybe because none of the respective functions/options has been called/set in the C-API before? Seems that this affects specific tests only that write and read back a binary, which is expected.

…vels

dcodeIO · 2018-01-12T17:16:47Z

test/binaryen.js/hello-world.js

@@ -45,7 +45,8 @@ module.dispose();

 // Compile the binary and create an instance
 var wasm = new WebAssembly.Instance(new WebAssembly.Module(binary), {})
-console.log(wasm); // prints something like "[object WebAssembly.Instance]"


changed this to evaluate the keys because output differs in node (looks like Instance { exports: ... })

dcodeIO · 2018-01-12T17:18:04Z

test/binaryen.js/hello-world.js.txt

@@ -24,8 +24,9 @@ optimized:
 )
 )

-binary size: 60
+binary size: 43


that's because the names section isn't generated by default now

kripken

Looks good to me with those docs.

I want to refactor those default values to another place, but I'll do that in a followup once I figure out where ;)

kripken · 2018-01-16T18:46:44Z

src/binaryen-c.cpp

@@ -70,6 +71,11 @@ Literal fromBinaryenLiteral(BinaryenLiteral x) {
 static std::mutex BinaryenFunctionMutex;
 static std::mutex BinaryenFunctionTypeMutex;

+// Optimization options
+static int optimizeLevel = BINARYEN_DEFAULT_OPTIMIZE_LEVEL;


this will have a single set of options for all Modules, globally. I guess that's ok, but we should document it.

kripken · 2018-01-16T18:47:02Z

src/binaryen-c.h

@@ -659,6 +660,24 @@ int BinaryenModuleValidate(BinaryenModuleRef module);
 // Runs the standard optimization passes on the module.
 void BinaryenModuleOptimize(BinaryenModuleRef module);

+// Gets the currently set optimize level. 0, 1, 2 correspond to -O0, -O1, -O2, etc.


we could document it here

kripken · 2018-01-17T00:57:45Z

lgtm, however when I run this locally, I get an error. It wants to change things this way:

$ git diff
diff --git a/test/binaryen.js/hello-world.js.txt b/test/binaryen.js/hello-world.js.txt
index a2b023d..6441f7e 100644
--- a/test/binaryen.js/hello-world.js.txt
+++ b/test/binaryen.js/hello-world.js.txt
@@ -26,7 +26,7 @@ optimized:
 
 binary size: 43
 
-[ 'exports' ]
-[ 'adder' ]
+
+adder
 
 an addition: 42

I would guess that is a printing difference between node and other shells? or is the output just not updated? (travis is slow for some reason, so i can't compare to the results there)

dcodeIO · 2018-01-17T01:15:36Z

Ah, seems printing an array to console looks different between shells (and some properties are not visible in mozjs). Latest commit should format it explicitly.

Followup to #1357. This moves the optimization settings into pass.h, and uses it from there in the various places. This also splits up huge lines from the tracing code, which put all block children (whose number can be arbitrarily large) on one line. This seems to have caused random errors on the bots, I suspect from overflowing a buffer. Anyhow, it's much more clear to split the lines at a reasonable length.

dcodeIO added 5 commits January 11, 2018 04:00

Add optimize, shrink level and debug info options to C/JS

eea6ec8

Fixes for 'var Binaryen' being renamed by Closure Compiler

d345b24

Add instantiate functionality for creating additional unique instance…

591e3b3

…s of the API

Use a workaround when running tests in node

3140519

Tests misuse a module as a script by concatenating, so instead of catching this case in the library, catch it there

Update sieve test

ced03d4

Seems optimized output changed due to running with optimize levels 2/1 now

dcodeIO added 3 commits January 11, 2018 18:48

Merge remote-tracking branch 'webassembly/master' into js-optimize-le…

4582bb3

…vels

Use the options with all pass runners

d6743a0

Update relooper-fuzz C-API test

abc0257

kripken reviewed Jan 11, 2018

View reviewed changes

dcodeIO added 3 commits January 11, 2018 22:13

Share defaults between tools and the C-API

30ef7df

Add a test for optimize levels

c77ace1

Unify node test support in check.by and auto_update_tests.py

526ec4f

dcodeIO added 9 commits January 11, 2018 23:21

Two blank lines

8252e06

No more than 79 cols

4456fec

learning python

d244d11

Fix kitchen-sink js test

cef8dcd

Also add getters for optimize levels and test them

badaae1

Also test debugInfo

def0b35

Add debug info to C tests that used it as well

85ad826

Fix missing NODEJS import in auto_update_tests

b88c9b6

Exclude wasm test cases when updating tests using node.js, for now

84d1186

dcodeIO force-pushed the js-optimize-levels branch from dd6d2ad to 84d1186 Compare January 12, 2018 01:30

dcodeIO added 3 commits January 12, 2018 02:44

Auto update tests

0d54da0

Merge remote-tracking branch 'webassembly/master' into js-optimize-le…

ed896c1

…vels

Detect node.js version (WASM support)

9d564b3

dcodeIO force-pushed the js-optimize-levels branch 2 times, most recently from 266b84a to ddccf29 Compare January 12, 2018 16:45

dcodeIO force-pushed the js-optimize-levels branch from ddccf29 to 5b14794 Compare January 12, 2018 16:50

the flake thing

b1deeb4

dcodeIO force-pushed the js-optimize-levels branch from 5b14794 to b1deeb4 Compare January 12, 2018 16:56

Update hello-world JS test (now also runs with node)

27238a9

dcodeIO commented Jan 12, 2018

View reviewed changes

feature-test WebAssembly in node instead

9e5c9ad

dcodeIO force-pushed the js-optimize-levels branch from 9945f5e to 9e5c9ad Compare January 12, 2018 17:43

dcodeIO added 3 commits January 12, 2018 19:03

sigh

6929c8f

rly

8370ce8

c'mon

14018d3

dcodeIO mentioned this pull request Jan 14, 2018

start function optimization AssemblyScript/assemblyscript#14

Closed

kripken approved these changes Jan 16, 2018

View reviewed changes

dcodeIO added 2 commits January 16, 2018 22:15

Document that these options apply globally, and where

228fce5

Fix punctuation

6ccf865

Make sure hello-world.js output doesn't differ between mozjs/node

d415b55

kripken merged commit 01b2398 into WebAssembly:master Jan 17, 2018

kripken mentioned this pull request Jan 18, 2018

Refactor optimization defaults #1366

Merged

dcodeIO mentioned this pull request Jan 18, 2018

Use Emscripten's MODULARIZE_INSTANCE when building binaryen.js #1368

Merged

Add optimize, shrink level and debug info options to C/JS #1357

Add optimize, shrink level and debug info options to C/JS #1357

Uh oh!

Conversation

dcodeIO commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcodeIO commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Jan 11, 2018

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcodeIO Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcodeIO Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcodeIO Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcodeIO commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jan 17, 2018

Uh oh!

dcodeIO commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dcodeIO commented Jan 11, 2018 •

edited

Loading

dcodeIO commented Jan 11, 2018 •

edited

Loading

dcodeIO Jan 11, 2018 •

edited

Loading

dcodeIO Jan 11, 2018 •

edited

Loading

dcodeIO Jan 11, 2018 •

edited

Loading

dcodeIO commented Jan 11, 2018 •

edited

Loading

dcodeIO commented Jan 17, 2018 •

edited

Loading