Remove noisy WebAssembly messages #5144
Conversation
These messages were getting printing to the console's error log even though they're not errors.
src/postamble.js
Outdated
| @@ -225,7 +225,7 @@ function run(args) { | |||
| if (preloadStartTime === null) preloadStartTime = Date.now(); | |||
|
|
|||
| if (runDependencies > 0) { | |||
| #if ASSERTIONS | |||
| #if ASSERTIONS && !BINARYEN_ASYNC_COMPILATION | |||
There was a problem hiding this comment.
i think we don't need this addition. when assertions are on, this can be shown. they aren't on in production. and assertions is our setting for 'do more checks and logging at runtime' (although maybe we should have a separate setting for logging, but that's a side issue)
There was a problem hiding this comment.
Maybe I misunderstand: when running the GCC torture tests the waterfall's JS files contain this printErr:
https://storage.googleapis.com/wasm-llvm/builds/linux/18611/wasm-torture-emwasm-18611.tbz2
IIUC ASSERTIONS isn't set?
https://github.com/WebAssembly/waterfall/blob/master/src/build.py#L1101
There was a problem hiding this comment.
We do enable assertions in -O0 builds (which should not be used in production). I guess the bot tests -O0?
In any case, we can probably put this behind a different flag. No existing flag seems suitable, so maybe we should just add one for runtime logging, RUNTIME_LOGGING (just need to add it in src/settings.js, then can use it).
There was a problem hiding this comment.
Well, my thinking for the change above was that it's never an error when we do binaryen async compilation, but it is a sign that something is wrong if we're not doing async compilation. Don't you think ASSERTIONS && !BINARYEN_ASYNC_COMPILATION expresses that combination properly?
There was a problem hiding this comment.
Not quite, since startup can be async for other reasons as well, in particular the mem init file for asm.js is loaded separately. That's an async operation that happens by default in -O2+. Also, if files are preloaded, that will make startup async as well.
Instead of trying to cover all those cases, I think it's simpler to just put logging like this behind another flag (as even in async startup, it's useful for debugging).
| @@ -7339,14 +7339,6 @@ def test_binaryen_invalid_method(self): | |||
| proc.communicate() | |||
| assert proc.returncode != 0 | |||
|
|
|||
| def test_binaryen_default_method(self): | |||
There was a problem hiding this comment.
i'm ok to remove this test (together with the logging removed in this pr) but it does verify that we emit native wasm by default, instead of the interpreter. i think it's worth verifying that. we could test it in another way, perhaps, but no great way seems obvious. maybe it's enough to see that when enabling the interpreter, the js in the build is massively bigger.
There was a problem hiding this comment.
Yeah it seems weird to me to have a test-related print only really there for a test.
There was a problem hiding this comment.
What do you think about putting the removed loggings behind RUNTIME_LOGGING, and using that in the test? Then we don't lose any coverage, and we've still gotten rid of all the annoying logging.
Oh, also please add yourself to AUTHORS if you're not there already.
|
I agree that I'd rather have no output at all when things go as expected. So I really don't think we need 'aysnchronously preparing', 'trying binaryen' and 'succeeded'. Those were useful when we were bringing things up, but are much less so now. Furthermore, now that async is the expected thing, I don't think we need 'asynchronously preparing wasm' and 'dependencies remain' because they are the expected behavior too. For testing the default binaryen method, can't we just make some assertion on the generated JS, rather than running it? |
|
Yeah, I suggested before we assert on including the interpreter leading to a huge size change. Otherwise hard to think of how to check for the interpreter (it doesn't have a comment inside saying what it is). |
|
So how about we go with the current patch, and figure out a test, if needed, elsewhere? |
|
I do think we still want those messages, just behind that new flag. While the default is async compilation, when figuring out a startup issue, the logging helps - it shows the order. If a crash happens, it's nice to know where it is, logically, as browser stack traces on async operations are not great. In my experience, we'd need to start adding in those same loggings manually, if we remove them, instead of just flipping the flag. As for the test, that's not urgent, so if you're going to get to it later I'm ok with removing this one. |
|
Are there missing changes for this patch? Or is it good to go? |
|
What I am suggesting is that the 3 messages in preamble.js that you erased be instead ifdefed on the new setting. |
|
Ah OK, here's an update. |
|
Thanks, looks good, leaving just the testing issue that we said would be a followup. |
These messages were getting printing to the console's error log even though they're not errors.