Check full JS output for hello world #12806
Conversation
tests/code_size/hello_world_wasm.js
Outdated
|
|
||
| function e(a) { | ||
| if (!a) return ""; | ||
| for (var k = a + NaN, b = a; !(b >= k) && f[b]; ) ++b; |
There was a problem hiding this comment.
This line keeps making me sad every day.. google/closure-compiler#3223
| var j = Math.sqrt; | ||
| var k = x.abort; | ||
| var l = NaN; | ||
| var m = Infinity; |
There was a problem hiding this comment.
Yikes - This looks like something that could be low hanging to DCE? I wonder why Closure did not clean them.. :/
There was a problem hiding this comment.
@alon... What phase would we expect to clean this up? Is it something that closure should be able deal with?
There was a problem hiding this comment.
I believe this is inside the wasm2js output. The JS optimizer doesn't normally run on that. We usually don't run a JS minifier on compiled code, as it could be massive. But you can try --closure 2 which I think does that.
An option is to handle this in wasm2js, that is, track what is needed and emit only that. Not sure how easy it would be.
There was a problem hiding this comment.
But that about closure compiler itself.. .shouldn't it be able to eliminate these unused variables?
There was a problem hiding this comment.
Closure would be able to do it., but closure does not run on the wasm2js output - except for --closure 2 (because, as I said, the compiled code can be massive).
There was a problem hiding this comment.
Ah I see. Thanks. I wonder if we should be running with --closure 2 for these minimal codesize tests? @juj?
There was a problem hiding this comment.
Because it falls down on huge files, I don't think we recommend --closure 2. But it would be good to investigate that, as maybe closure has gotten faster/uses less memory now?
|
lgtm |
|
Are you OK with this @kripken ? If it proves too fragile we can always revert.. |
For the smallest hello world test, don't just check the output size but check the full JS output matches the expectations. That means that any change that touches those core lines of output will need to rebaseline this test. However: a) such changes deserve extra scrutiny b) such changes should be few and far between c) rebaselining is trivial (just run with EMTEST_REBASELINE=1) Note that we do not compare the full wasm output since that is even more fragile and can change with LLVM updates.
sbc100
force-pushed
the
full_output_check
branch
from
b71df15 to
eeb8998
Compare
| var j = Math.sqrt; | ||
| var k = x.abort; | ||
| var l = NaN; | ||
| var m = Infinity; |
There was a problem hiding this comment.
I believe this is inside the wasm2js output. The JS optimizer doesn't normally run on that. We usually don't run a JS minifier on compiled code, as it could be massive. But you can try --closure 2 which I think does that.
An option is to handle this in wasm2js, that is, track what is needed and emit only that. Not sure how easy it would be.
|
It would actually be very good to run |
For the smallest hello world test, don't just check the output size
but check the full JS output matches the expectations.
That means that any change that touches those core lines of output
will need to rebaseline this test. However:
a) such changes deserve extra scrutiny
b) such changes should be few and far between
c) rebaselining is trivial (just run with EMTEST_REBASELINE=1)
Note that we do not compare the full wasm output since that is even
more fragile and can change with LLVM updates.