Fix for undefined ASSERTIONS variable in html output of emcc

[nazar-pc]

It seems to be specific to HTML builds, since src/arrayUtils.js doesn't define ASSERTIONS anymore (https://github.com/kripken/emscripten/pull/5720/files#diff-86414f2ed951321c81d9a4aa55bb139cL13).

Also removed comment (left from the PR linked above): it indicated that previously the code didn't work at all, but should start working with this PR.

@buu700, did I interpret it correctly?

This is a response to 93479ec#commitcomment-25492064

[buu700]

Cool, that solution looks good to me, and seems to work as described for me locally.

As far as the comment, I think you may have misinterpreted it, but this diff from when it was added should clear up the meaning:

tl;dr: One function with a simple #if ASSERTIONS had to be copied and pasted to temporarily work around the lack of a solution for invoking the preprocessor from Python, which we'll want to remember to revert when that is no longer the case.

[nazar-pc]

But now having var ASSERTIONS on higher level the comment is not needed anymore, right?

[buu700]

That comment was about intArrayToString being duplicated (the two function bodies in the ternary are identical aside from the assertion on line 20), not about var ASSERTIONS, so this change doesn't have anything to do with the comment.

It seems like it could be a lot more clear, though. Maybe change it to something like // TODO: After support is added for invoking the preprocessor from Python, use it on this file in emcc.py's generate_html and collapse intArrayToString into a single function with an #if directive.

[nazar-pc]

Do we need #if now though? Is that function expected to be called very often?
If not, I can refactor it to be a single function with regular JS if condition. Also Closure Compiler will unwrap if when ASSSERTIONS variable is a constant anyway, so for main JavaScript build in production there will be no cost at all.

[nazar-pc]

Well, it is an exceptional situation anyway, so I went ahead and simplified it even further to single function. Shouldn't make any real-world difference even without Closure Compiler post-processing.

Does it make sense?

[buu700]

I'd originally written it pretty much the same way as you're suggesting, but @kripken specifically requested duplicating it: #5296 (comment)

That is an interesting point about Closure. I believe Closure output requires access to eval at run-time, though, unless either I'm mistaken or that's changed, in which case it's a nonstarter for many use cases since most Content Security Policies don't whitelist unsafe-eval and shouldn't be assumed to be used for every production build. (Am I correct on that? I'd love to be able to start using --closure.)

---

Edit: Since that link doesn't seem to work, here's the relevant comment:

The real problem is here though. This adds a function call at runtime for every iteration in the loop, which if it isn't optimized by the JIT, will make intArrayToString much slower, and we do care about speed there.

The only way around that is preprocessing, which we now want to avoid - so we need something else creative to get around this. Maybe just two copies of the code wouldn't be too bad.

[nazar-pc]

Am I correct on that? I'd love to be able to start using --closure

I don't think it ever requires eval. All of the projects I've been using it with didn't have any eval added by Closure Compiler. Also here is a pretty big library compiled with Emscripten and Closure Compiler enabled and all 3 eval come from Emscripten itself: https://github.com/nazar-pc/noise-c.wasm/blob/master/noise-c.js
Also I think it should be possible to eliminate some of remaining eval() calls introduced by Emscripten too, will work on it separately.

If that was a performance concern, I've just prepared benchmark that compares performance of the example with if inside and without it: https://jsfiddle.net/fanLn4k4/

My Chromium Nightly and Firefox Nightly do not see any measurable difference. So either it is not the case at all or it was fixed in V8 and other JavaScript engines already.

[buu700]

Awesome. I was just speaking about --closure specifically and didn't expect that Closure Compiler itself would introduce evals, but wasn't actually sure, so that's great to hear that you might be able to remove the evals from emscripten's output.

As far as the if, any thoughts @kripken? (It's at least an improvement over when I'd added a function invocation overhead in the form of maybeAssert.)

[kripken]

Yeah, this seems good. The benchmark doesn't actually reach the ASSERTIONS check (the variable isn't defined, so it would throw, unless I'm missing something), but that's fine as it's the expected case anyhow. So perhaps we were overly cautious before.

[kripken]

Looks good in local testing too. Just waiting on CI.