Leverage coalescing operators. NFC

[RReverser]

Note that this bumps minimum engine versions that can avoid transpilation.

I believe this is not blocked on #11984 as it doesn't require Closure to emit runtime helpers.

This is mostly automated with ast-grep, bunch of custom rules, and a few manual fixups. Best viewed with "ignore whitespace changes" (https://github.com/emscripten-core/emscripten/pull/20531/files) due to indentation changes and due to first commit being #20530.

Care was taken to ensure that false-y conditions still use || where it might matter and only null-ish conditions use ??.

[RReverser]

Sigh, does that failure mean it would require Node.js update too? I'm guessing that's less likely?

[sbc100]

This is awesome!

Can you update test_es5_transpile to make sure this transpiles cleanly with closure copiler?

Normally/historically when I make this kind of change I like to land the change the bumps to browser versions and updates test_es5_transpile (and possibly introduces just one usage of the new construct into the stdlib) first.. and then to the large scale change as a followup. I think this helps with review and makes bisecting easier.

[sbc100]

Sigh, does that failure mean it would require Node.js update too? I'm guessing that's less likely?

Old versions of node (just like old browser versions) should be supported via transpilation, no?

[RReverser]

Old versions of node (just like old browser versions) should be supported via transpilation, no?

Probably. I'm not entirely sure what this test_node_compat job does, never looked at it before, I just see that it fails because it seems to use rather old Node.

[RReverser]

Normally/historically when I make this kind of change I like to land the change the bumps to browser versions and updates test_es5_transpile (and possibly introduces just one usage of the new construct into the stdlib) first.. and then to the large scale change as a followup. I think this helps with review and makes bisecting easier.

That makes sense, probably helps with ignoring revisions too. I'm happy to split out if the PR is looking acceptable otherwise, but first would probably need to figure out this failure.

[RReverser]

I'm not even sure why I'm doing this. I don't need it for my work, but this kind of large-scale refactorings is somewhat addicting 😅

[RReverser]

...huh, those are some weird "bulk memory support is not enabled" failures. Especially given that it's couple of standalone tests that are failing - those that don't depend on JS.

[RReverser]

Probably. I'm not entirely sure what this test_node_compat job does, never looked at it before, I just see that it fails because it seems to use rather old Node.

/home/circleci/node-v10.19.0-linux-x64/bin/node /home/circleci/project/src/compiler.js /tmp/emtest_d5kxfnv8/tmpg3hc1hda.json --symbols-only

Just noticed the version, that's very old Node indeed. Why does that job use it instead of the one from emsdk which I believe is newer?

[RReverser]

Can you update test_es5_transpile to make sure this transpiles cleanly with closure copiler?

Normally/historically when I make this kind of change I like to land the change the bumps to browser versions and updates test_es5_transpile (and possibly introduces just one usage of the new construct into the stdlib) first.. and then to the large scale change as a followup. I think this helps with review and makes bisecting easier.

Extracted in #20549. Interesting that I can observe some failures even on that innocently-looking change.

[RReverser]

@sbc100 Given that other PRs were merged, why is test-node-compat still breaking on ?. as unknown operator? Is it because it's still using old Node w/o transpilation?

[sbc100]

In my PR where I tried to do something like this I ended up doing #19772

    if not feature_matrix.caniuse(feature_matrix.Feature.OPTIONAL_CHAINING):
      settings.TRANSPILE_TO_ES5 = True

You can test this locally be setting NODE_JS_TEST to v10.19.0. I have the following in my .emscripten so I can switch between different versions of node easily:

OLDEST_NODE = '/usr/local/google/home/sbc/bin/node-v10.19.0-linux-x64/bin/node'  
NEW_NODE = '/usr/local/google/home/sbc/bin/node-v19.3.0-linux-x64/bin/node'         
EMSDK_NODE = [os.path.expanduser('~/dev/wasm/emsdk/node/16.20.0_64bit/bin/node')]
                                                                                 
#NODE_JS = NEW_NODE                                                              
#NODE_JS = OLDEST_NODE                                                           
NODE_JS = EMSDK_NODE                                                             
                                                                                 
#NODE_JS_TEST = '/usr/local/google/home/sbc/bin/node-v21.0.0-v8-canary202309143a48826a08-linux-x64/bin/node'
NODE_JS_TEST = EMSDK_NODE                                                        
#NODE_JS_TEST = '/usr/local/google/home/sbc/bin/node-v18.11.0-linux-x64/bin/node'
#NODE_JS_TEST = '/usr/local/google/home/sbc/bin/node-v17.6.0-linux-x64/bin/node' 
#NODE_JS_TEST = '/usr/local/google/home/sbc/bin/node-v15.14.0-linux-x64/bin/node'
#NODE_JS_TEST = '/usr/local/google/home/sbc/bin/node-v16.18.0-linux-x64/bin/node'
#NODE_JS_TEST = [os.path.expanduser('~/bin/node-v18.0.0-v8-canary2022031068abd7e7cd-linux-x64/bin/node')]

JS_ENGINES = [NODE_JS_TEST, V8_ENGINE]

Then I just uncomment the NODE_JS_TEST that I want to test with.

[RReverser]

Oh wow, I never saw that PR, sorry for stepping on your toes with this one. Yeah, using feature matrix for this makes sense.

I could probably also include BCD syntax data in my automatic feature detection PR too.

[sbc100]

Oh wow, I never saw that PR, sorry for stepping on your toes with this one. Yeah, using feature matrix for this makes sense.

No worries! I half forgot about it myself.. I have so many stale PRs I loose track.

I could probably also include BCD syntax data in my automatic feature detection PR too.

[RReverser]

I could probably also include BCD syntax data in my automatic feature detection PR too.

Did you mean to write something after that quote?

[RReverser]

So close... but other.test_closure_type_annotations looks like a real failure. Comparing output JS saved via --save-dir, it does look like a lot of code that was previously DCEd is now retained.

I'm not sure what's causing it yet. That test shouldn't go via transpiling, so that's not the cause, but the only other change is usage of ?. operators... maybe Closure can't optimise them yet?

[RReverser]

Doesn't solve the issue, but this construction in the test looks odd btw:

emscripten/test/test_other.py

Lines 9274 to 9275 in 5137bfc

	if 'my ' + attribute in code:
	self.assertFalse('Attribute `%s` could not be DCEd' % attribute)

assertFalse takes an expression as its first argument, but this is passing a message there instead. if expr on the message still works, but maybe a better way to write it would be

self.assertFalse('my ' + attribute in code, 'Attribute `%s` could not be DCEd' % attribute)

[sbc100]

Doesn't solve the issue, but this construction in the test looks odd btw:

emscripten/test/test_other.py

Lines 9274 to 9275 in 5137bfc

	if 'my ' + attribute in code:
	self.assertFalse('Attribute `%s` could not be DCEd' % attribute)

assertFalse takes an expression as its first argument, but this is passing a message there instead. if expr on the message still works, but maybe a better way to write it would be

self.assertFalse('my ' + attribute in code, 'Attribute `%s` could not be DCEd' % attribute)

Probably historical reasons.

And using f-strings would be even better.

[RReverser]

I'm not sure what's causing it yet. That test shouldn't go via transpiling, so that's not the cause, but the only other change is usage of ?. operators... maybe Closure can't optimise them yet?

Testing a bit further and comparing diffs with --closure-args=--debug added to the test, this seems like the root problem. Some slight change in semantics is causing Closure to keep all this code.

That's pretty unfortunate and I don't see where to go from here, other than maybe reporting issue to them.

[sbc100]

I don't quite understand the reason that other.test_closure_type_annotations would start failing? Is it due to the change made to the JS library input code?

[sbc100]

Maybe you can land this change on its own / separately?

[RReverser]

Is it due to the change made to the JS library input code?

Yeah looks like at least some of the changes cause Closure to retain variables that would be previously DCEd. Looking at the diff, those are huge chunks of SDL, WebGL and HTML5 API bindings that were previously removed. But there's nothing special about changes to those libraries in particular...

[RReverser]

Another offtopic, but you landed that PR with automatic CI retries on failure, right? Do you know why those flaky tests are still failing then?

[sbc100]

Another offtopic, but you landed that PR with automatic CI retries on failure, right? Do you know why those flaky tests are still failing then?

There are two mechanisms for re-trying... tests marked a @flakey explictly are re-tried if they fail. We have very few such tests and non of the browser tests are yet marked in this way.

However because browser tests are just generally more flakey we have a second mechanism that retries all browser tests then they fail.

However, I believe the most of the failures we are seeing in the browser test suite today involve the browser crashing or otherwise becoming unresponsive.. which then results in all subsequest browser tests failing with "[no http server activity]". I don't believe any amount of re-trying will help in this case. I think we need to somehow restart the browser when this happens, but we don't currently have a mechanism for this. CC @kripken

[RReverser]

Slowly went through reverting bunch of files at a time and narrowed issue down to changes in just two files

src/library_browser.js
src/library_webgl.js

Now I'm going to try and identify what exactly is "rooting" those functions and prevents them from being DCEd.

[RReverser]

Huh, that's... not at all the kind of changes I expected to be problematic.

diff --git a/src/library_browser.js b/src/library_browser.js
index 67accc101..6e5904b1a 100644
--- a/src/library_browser.js
+++ b/src/library_browser.js
@@ -736,7 +736,7 @@ var LibraryBrowser = {
     {{{ runtimeKeepalivePush() }}}
 
     var _suffix = UTF8ToString(suffix);
-    Browser.asyncPrepareDataCounter ||= 0;
+    if (!Browser.asyncPrepareDataCounter) Browser.asyncPrepareDataCounter = 0;
     var name = 'prepare_data_' + (Browser.asyncPrepareDataCounter++) + '.' + _suffix;
     var cname = stringToNewUTF8(name);
     FS.createPreloadedFile(
diff --git a/src/library_webgl.js b/src/library_webgl.js
index b4cde3c73..6c26b45fc 100644
--- a/src/library_webgl.js
+++ b/src/library_webgl.js
@@ -231,7 +231,9 @@ for (/**@suppress{duplicate}*/var i = 0; i < {{{ GL_POOL_TEMP_BUFFERS_SIZE }}};
     // cleared the stored error by a call to glGetError().
     recordError: function recordError(errorCode) {
 #if GL_TRACK_ERRORS
-      GL.lastError ||= errorCode;
+      if (!GL.lastError) {
+        GL.lastError = errorCode;
+      }
 #endif
     },
     // Get a new ID for a texture/buffer/etc., while keeping the table dense and

Reverting just those 2 changes is enough to make test pass. This feels almost random :/

[sbc100]

Huh, that's... not at all the kind of changes I expected to be problematic.

diff --git a/src/library_browser.js b/src/library_browser.js
index 67accc101..6e5904b1a 100644
--- a/src/library_browser.js
+++ b/src/library_browser.js
@@ -736,7 +736,7 @@ var LibraryBrowser = {
     {{{ runtimeKeepalivePush() }}}
 
     var _suffix = UTF8ToString(suffix);
-    Browser.asyncPrepareDataCounter ||= 0;
+    if (!Browser.asyncPrepareDataCounter) Browser.asyncPrepareDataCounter = 0;
     var name = 'prepare_data_' + (Browser.asyncPrepareDataCounter++) + '.' + _suffix;
     var cname = stringToNewUTF8(name);
     FS.createPreloadedFile(
diff --git a/src/library_webgl.js b/src/library_webgl.js
index b4cde3c73..6c26b45fc 100644
--- a/src/library_webgl.js
+++ b/src/library_webgl.js
@@ -231,7 +231,9 @@ for (/**@suppress{duplicate}*/var i = 0; i < {{{ GL_POOL_TEMP_BUFFERS_SIZE }}};
     // cleared the stored error by a call to glGetError().
     recordError: function recordError(errorCode) {
 #if GL_TRACK_ERRORS
-      GL.lastError ||= errorCode;
+      if (!GL.lastError) {
+        GL.lastError = errorCode;
+      }
 #endif
     },
     // Get a new ID for a texture/buffer/etc., while keeping the table dense and

Reverting just those 2 changes is enough to make test pass. This feels almost random :/

I little worrying.. but this change lgtm still

[RReverser]

but this change lgtm still

Hm are you sure, despite the bloat it potentially adds?

[RReverser]

Should I remove the test then or?

[sbc100]

To be clear those changes break closure's ability to DCE other unrelated stuff? WTF!

[sbc100]

Should I remove the test then or?

I think you should just revert those parts of the change.

[RReverser]

to DCE other unrelated stuff

Somewhat related stuff (e.g. that GL.lastError change seems to keep parts of WebGL that the tests checks again), but yeah still WTF is the right reaction.

I think you should just revert those parts of the change.

FWIW even without them manual verification shows that the output JS is still much larger than before this PR. It's just that it passes the specific checks in the test.

[RReverser]

Trying with Closure, it seems it treats those operators somewhat differently when using globals.

Standalone sample:

globalThis.foo = () => {
  if (!GL.lastError) {
    GL.lastError = errorCode;
  }
};

globalThis.bar = () => {
  GL.lastError ||= errorCode;
}

Pretty-printed output after advanced opts:

globalThis.h = function() {
  GL.lastError || (GL.lastError = errorCode);
};
globalThis.g = function() {
  var a;
  (a = GL).lastError || (a.lastError = errorCode);
};

I think Closure is "worried" that the globals might happen to be getters on the global object, in which case they might have side effects, so they need to be stored in a separate variable first to ensure they're accessed only once when using ||=, and that, in turn, breaks some optimisations.

If this assumption is correct, it would explain why the issue occurs only when accessing properties on our own Browser and GL variables, which become globals in the default Emscripten's output.

I wonder if there's some way to annotate those variables as safe to access for Closure.

[sbc100]

Trying with Closure, it seems it treats those operators somewhat differently when using globals.

Standalone sample:

globalThis.foo = () => {
  if (!GL.lastError) {
    GL.lastError = errorCode;
  }
};

globalThis.bar = () => {
  GL.lastError ||= errorCode;
}

Pretty-printed output after advanced opts:

globalThis.h = function() {
  GL.lastError || (GL.lastError = errorCode);
};
globalThis.g = function() {
  var a;
  (a = GL).lastError || (a.lastError = errorCode);
};

I think Closure is "worried" that the globals might happen to be getters on the global object, in which case they might have side effects, so they need to be stored in a separate variable first to ensure they're accessed only once when using ||=, and that, in turn, breaks some optimisations.

If this assumption is correct, it would explain why the issue occurs only when accessing properties on our own Browser and GL variables, which become globals in the default Emscripten's output.

I wonder if there's some way to annotate those variables as safe to access for Closure.

This makes me wonder if we need more code size tests to catch this kind of thing.. perhaps one that does -sINCLUDE_FULL_LIBRARY?

How about landing the just the functional part of this change first without that actual mass usage of the new operators? (Or perhaps just one usage). Then the large scale change can follow once we figure out the closure stuff?

[RReverser]

perhaps one that does -sINCLUDE_FULL_LIBRARY?

That's what the failing test does at the moment. Although maybe a size test for INCLUDE_FULL_LIBRARY would also be good.

How about landing the just the functional part of this change first without that actual mass usage of the new operators?

Main functional part has already landed in #20549, the only remaining functional bit is transpilation versions but it's pretty tiny.

I first want to see if it's possible to either annotate those few vars somehow or just revert those files in particular for now.

I tried adding $Browser__docs: '/** @const */', but that reveals a bunch of other issues with function signature mismatch and whatnot inside library_browser.js, so looking for a less impactful solution for now.

[RReverser]

Ok I applied just the simple fix to those methods. Idk what to do about Closure issue in general, but since you're okay with it and the increased size of INCLUDE_FULL_LIBRARY is not covered by tests, I suppose it should be fine.