Remove supportsWebGL2EntryPoints #9919

juj · 2019-11-28T09:53:02Z

Remove support for WebGL 2 in Chrome 57 due to its high maintenance burden on code size. Reduces -15.9% of code size (997 bytes) in https://github.com/juj/webgl_render_test for WebGL 2. Introduce new setting -s OLDEST_SUPPORTED_CHROME_VERSION=x to enable customizing which Chrome versions to target.

juj · 2019-11-28T09:57:44Z

CC @kenrussell @kainino0x

juj · 2019-11-28T10:50:21Z

Argh, now I realize can't quite leverage the full potential until we have a setting to opt out from WebGL1 support altogether. I was hesitating to add a -s USE_WEBGL1=0 option since #7612 started along similar lines by adding an option for that as a GL_MIN_FEATURE_LEVEL.

Until that happens, this PR can make a partial step towards that though.

VirtualTim · 2019-11-29T01:45:20Z

Chrome 69 for desktop has a 0.4% market share. No idea what the Chrome 57 market share is, but I imagine it's at least 10% of that.

I think it would be crazy to try and support such old versions of Chrome, especially at the detriment of other users. Not to mention the maintenance burden.

So I'm definitely in support of PRs like this 👍.

juj · 2019-12-02T11:44:53Z

@kripken: how do you feel about the -s OLDEST_SUPPORTED_CHROME_VERSION=x setting? I am thinking of adding a similar set of OLDEST_SUPPORTED_CHROME_VERSION, OLDEST_SUPPORTED_FIREFOX_VERSION, OLDEST_SUPPORTED_IE_VERSION, OLDEST_SUPPORTED_SAFARI_VERSION (and perhaps also needs OLDEST_SUPPORTED_IOS_SAFARI_VERSION separately) as needed, that would enable sunsetting a lot of these kind of backwards compatibility items in a nice fashion where we won't just remove compatibility work that we have done, but allow people to gracefully drop old stuff for smaller code size?

juj · 2019-12-02T11:45:18Z

For another example, e1cab71

juj · 2019-12-02T12:15:09Z

Posted PR #9937 to illustrate the idea. Some people want to target ancient browsers, others want to rely on the newest and greatest. This would allow catering to everyone.

Also later under the hood we could adjust limits automatically, e.g. when targeting Wasm, that would imply Edge >= 16, Firefox >= 52, Chrome >= 57, Safari >= 11 and IE not supported.

sbc100 · 2019-12-02T15:09:44Z

I think I like this idea. Presumably we could use the default setting of each of these to document an absolute lower bound on each, and move that forward over time as we completely remove support for certain older browsers.

The main downside is that it adds N more settings to our already numbers configuration matrix. Is there some way we could matrix dimensionality somehow while still being intuitive, or is one setting per browser the only logical way to express this? i.e. would a single ES version, or year not be granular enough?

juj · 2019-12-02T17:17:08Z

i.e. would a single ES version, or year not be granular enough?

The reason why I like this browser version specific check is that that matches most often with what people are thinking about. That is, users report bugs that "did not work on IE11/Chrome 76/Safari 11.0.1", and devs make decisions "we don't support IE" etc.

Also I recommend this kind of check is kept light and more or less orthogonal. For example, I don't recommend we start building up complex features <-> browsers mappings and validations, so e.g. adding all kinds of if (-s wasm=1 && -s oldest_ie_version <= 11) fatal_error('wasm doesn't work on ie11'); gates would be just somewhat tedious and not that useful.

I intend these checks are more like an easier way to remove cruft in JS libraries, for example I'd like a way to express when I'd be able to remove mozMovementX here or webkitRequestAnimationFrame here in a way that is natural and won't cause people to yell if they wished to still have that, and in a way that won't need extra -s settings added for each single thing.

Also note that I don't think these OLDEST_SUPPORTED_X_VERSION fields would make feature-specific fields and larger workarounds redundant. For example it is still useful to have -s GL_DISABLE_HALF_FLOAT_EXTENSION_IF_BROKEN=0/1 setting be separate from OLDEST_SUPPORTED_SAFARI_VERSION, because people may want to target old Safari versions, but they do not care about half float extension, and might want to turn that off. However the setting OLDEST_SUPPORTED_SAFARI_VERSION can safely control the value of -s WORKAROUND_IOS_9_RIGHT_SHIFT_BUG=0/1` since practically without that Safari 9 generated asm.js code is in very high risk of being broken.

Also I think these fields should obsolete LEGACY_VM_SUPPORT=0/1 option as a more fine grained and specific setting to control, and they could also obsolete -s ENVIRONMENT=shell settings if we add -s OLDEST_SUPPORTED_NODE_VERSION=x instead, so users could specify -s OLDEST_SUPPORTED_NODE_VERSION=-1 or -s OLDEST_SUPPORTED_NODE_VERSION=None to disable support for Node.js.

kripken · 2019-12-03T17:53:51Z

I think adding fine-grained options like this is ok, but I think we should also keep LEGACY_VM_SUPPORT (which might just flip on the fine-grained flags, and that's it). That's useful for a user that knows they want to support ancient browsers as much as possible, but hasn't actually done an investigation to find the specific versions.

kripken · 2019-12-03T17:55:02Z

src/settings.js

+// contain polyfills and backwards compatibility for Chrome >= 58 and newer. Set this to a small value to target old browsers, and to a high value to reduce code size
+// by dropping polyfill support for older browsers. Default to 0 to mean "all Chrome versions" (although in practice a large number of features require a specific much
+// newer Chrome version, e.g. Wasm or WebGL 2)
+var OLDEST_SUPPORTED_CHROME_VERSION = 0;


How about MIN_CHROME_VERSION?

More importantly, I think this should have a reasonable default, which is to not support super-old browsers. Like how LEGACY_VM_SUPPORT is optional, and not the default. We shouldn't by default suffer too much for ancient browsers.

However, as this is the actual version, that implies we'd need to update it from time to time. Maybe that's not so bad. Anyhow, as a starting version, it might be 58 as that is a useful value here.

I think perhaps leave tuning the initial defaults for later once all of them have landed, and when LEGACY_VM_SUPPORT can migrate to preconfiguring OLDEST_SUPPORTED_X_VERSION values?

MIN_CHROME_VERSION sounds ok too, I'll rename to that.

I think it might be more disruptive to change MIN_CHROME_VERSION from 0 to 58 (or such) later, because it kind of changes the actual use and meaning of the flag - from "optionally change this to not support old browsers" to "optionally change this to yes support old browsers".

I would prefer if the default option were to only support "current" Chrome versions - that is, the last ~3-6 months of Chrome releases (relative to the release date of a version of Emscripten). Realistically, how sure are we even that the rest of Emscripten works on browsers that old? - given, I assume, most developers are not actively testing that far back. And there are very few users on old Chrome versions (<1%).

Maybe this is not in line with the philosophy of the project. I have run across quite a bit of code in Emscripten to handle old browsers, which I would like to remove if it were possible.

Anyway, if you don't want to add that, then defaulting to 58 (now) would be an OK option to me.

Realistically, how sure are we even that the rest of Emscripten works on browsers that old?

I target Samsung Galaxy S2 and Samsung Galaxy Nexus with Chrome 33, iPhone 4S with Safari 8.0, and Raspberry Pi Zero underclocked to 200MHz as the lowest end devices, mostly because they are extremely good performance profiling targets for tiny 2D "Candy Crush" games (and run at 30-60fps). Emulations are needed for performance.now(), requestAnimationFrame(), fonts, landscape/portrait rotation and especially audio, but things work well on those even.

Most of these tiny playable games are on the order of NES/C64 level of graphical complexity, and if you wanted to run an ad as a playable using asm.js/wasm with Emscripten, you'd really like it to tax only a few % of your browser's gigahertz scale CPU capacity, so it is an useful exercise to look at scaling to ancient browsers.

The only hard limit I have seen so far is on Android 4.3 and older with built-in browser AppleWebKit/534.30 and older, for some reason that completely misparses and misexecutes asm.js code which I was never able to figure out. There are also unfortunate issues with new Chrome and Firefox versions, where sometimes the old built-in browser that ships with a device works better than newest Chrome or Firefox, understandably due to expired lack of testing.

kripken · 2019-12-03T18:13:59Z

tests/test_other.py

@@ -9509,6 +9509,7 @@ def test_minimal_runtime_code_size(self):
                               '-s', 'GL_SUPPORT_EXPLICIT_SWAP_CONTROL=0',
                               '-s', 'GL_POOL_TEMP_BUFFERS=0',
                               '-s', 'FAST_UNROLLED_MEMCPY_AND_MEMSET=0',
+                               '-s', 'OLDEST_SUPPORTED_CHROME_VERSION=58',


If we switch to 58 as suggested earlier as the default value, this line can be removed.

kripken · 2019-12-03T18:14:20Z

tests/test_other.py

@@ -9531,7 +9532,7 @@ def test_minimal_runtime_code_size(self):
      (opts, hello_world_sources, {'a.html': 968, 'a.js': 604, 'a.wasm': 86}),
      (asmjs + opts, hello_webgl_sources, {'a.html': 881, 'a.js': 4918, 'a.asm.js': 11139, 'a.mem': 321}),
      (opts, hello_webgl_sources, {'a.html': 857, 'a.js': 4874, 'a.wasm': 8841}),
-      (opts, hello_webgl2_sources, {'a.html': 857, 'a.js': 5507, 'a.wasm': 8841}) # Compare how WebGL2 sizes stack up with WebGL 1
+      (opts, hello_webgl2_sources, {'a.html': 857, 'a.js': 5362, 'a.wasm': 8841}) # Compare how WebGL2 sizes stack up with WebGL 1


wow, that was a big cost for backwards compatibility...

sbc100 · 2019-12-03T19:28:50Z

I think this change OK to land, but I do think we risk making testing harder. Since the stated purpose is to be able to strip the JS code using the equivalent of ifdefs it is not going to be possible for us to test all combinations.

Perhaps if we come up with simple testing strategy that we can follow. Perhaps something like: for each location where we modify generated JS code based on a specific browser version we need a test for that part of the code in both configurations. ?

kainino0x · 2019-12-03T22:52:55Z

src/library_webgl.js

@@ -526,6 +526,18 @@ var LibraryGL = {
      webGLContextAttributes['preserveDrawingBuffer'] = true;
 #endif

+#if USE_WEBGL2 && OLDEST_SUPPORTED_CHROME_VERSION <= 57
+      // BUG: Workaround Chrome WebGL 2 issue: the first shipped versions of WebGL 2 in Chrome did not actually implement the new WebGL 2 functions.


I know this comment is old, but can we elaborate in the comment? I'm pretty sure (cab6be0) this was about the srcOffset+length versions of various entry points which were added to WebGL 2 (possible added to the spec after Chrome shipped, I don't remember). And were there really no versions of Firefox that shipped WebGL 2 without these entry points?

Firefox did ship with the new entry points from the first version of WebGL 2 forward. It was only Chrome that missed the new entry points.

I know this comment is old, but can we elaborate in the comment?

check.

src/library_webgl.js

kainino0x · 2019-12-03T23:14:35Z

src/settings.js

+// contain polyfills and backwards compatibility for Chrome >= 58 and newer. Set this to a small value to target old browsers, and to a high value to reduce code size
+// by dropping polyfill support for older browsers. Default to 0 to mean "all Chrome versions" (although in practice a large number of features require a specific much
+// newer Chrome version, e.g. Wasm or WebGL 2)
+var OLDEST_SUPPORTED_CHROME_VERSION = 0;


I would prefer if the default option were to only support "current" Chrome versions - that is, the last ~3-6 months of Chrome releases (relative to the release date of a version of Emscripten). Realistically, how sure are we even that the rest of Emscripten works on browsers that old? - given, I assume, most developers are not actively testing that far back. And there are very few users on old Chrome versions (<1%).

Maybe this is not in line with the philosophy of the project. I have run across quite a bit of code in Emscripten to handle old browsers, which I would like to remove if it were possible.

Anyway, if you don't want to add that, then defaulting to 58 (now) would be an OK option to me.

kripken · 2019-12-04T00:23:05Z

@kainino0x

That link is for desktop only - is there public data on mobile? (modifying the graph there doesn't seem to support that) I worry about older Android versions with non-updating Chrome, stuff like that.

But I agree in general - supporting 99% of Chrome users by default seems fine, and to get really old legacy versions, a flag would be needed. That's basically what LEGACY_VM_SUPPORT is for today, but really it's mostly useful for non-chrome browsers, where legacy versions are more of an issue (e.g. firefox has ESR, there is still a significant amount of IE11, etc.).

kainino0x · 2019-12-04T00:44:15Z

I don't have any stats offhand. Unfortunately for some reason that site doesn't track browser versions on mobile.

Old Android devices that stop getting Android updates do continue getting Chrome updates for quite a long time as long as they're using the Play Store. Last I heard Chrome still supports back to Android 4.4 (2013), though it could have advanced slightly since then.

sbc100 · 2019-12-04T01:17:43Z

My understanding matches Kai's, and I agree we should set sensible defaults here and not try to support older browser with a tiny userbase (not by default anyway). Hopefully we can use real usage number to drive this.

kainino0x · 2019-12-04T01:34:35Z

Let's try to get Chrome telemetry (UMA) data about this. I don't know how to query UMA to get the data we need, so one of us will have to try to figure it out.

VirtualTim · 2019-12-04T01:37:46Z

For my use case dropping older browsers would be great. I'm using pthreads, which depends on SharedArrayBuffer + Atomics, so I can't support older browsers anyway.
I realise that this isn't the case for everyone, but for me Emscripten code to support older browsers is a waste. So any options to remove that would be beneficial.

juj · 2019-12-04T10:14:25Z

Anyway, if you don't want to add that, then defaulting to 58 (now) would be an OK option to me.

If we switch to 58 as suggested earlier as the default value, this line can be removed.

That's basically what LEGACY_VM_SUPPORT is for today, but really it's mostly useful for non-chrome browsers, where legacy versions are more of an issue (e.g. firefox has ESR, there is still a significant amount of IE11, etc.).

I really dislike the whole LEGACY_VM_SUPPORT setting, that is an inflexible "canned decision" from compiler vendors' part to draw a fixed boundary between what is old and what is new. With the exact same effort, we can do more appropriate checks, since the data is readily available on caniuse.com and MDN, and we need to research that anyways when doing the backwards compatible code. So might as well add it in the repository in a form that is "pre-researched" for users as well.

That is why I think we should just remove LEGACY_VM_SUPPORT altogether. It does not serve the user, because I don't think there really exists a binary "I want to support old things" or "I don't want to support old things" choice by the developer, but they need to define what "old" means for them, and then they need to cross-reference and research whether their understanding of "old" matches LEGACY_VM_SUPPORT's understanding of old.

That is why I think these browser version specific checks are more exact and accurate, and people can then in straightforward fashion add cmdline flags that match what they are thinking. "I don't support IE -> -s MIN_IE_VERSION=-1".

But @sbc100 does raise a good point:

I think this change OK to land, but I do think we risk making testing harder. Since the stated purpose is to be able to strip the JS code using the equivalent of ifdefs it is not going to be possible for us to test all combinations.

Perhaps if we come up with simple testing strategy that we can follow. Perhaps something like: for each location where we modify generated JS code based on a specific browser version we need a test for that part of the code in both configurations. ?

Writing code for

#if MIN_IE_VERSION <= 8
   {{{ makeSetValue('width', '0', 'rect.right - rect.left', 'double') }}};
   {{{ makeSetValue('height', '0', 'rect.bottom - rect.top', 'double') }}};
#else
   {{{ makeSetValue('width', '0', 'rect.width', 'double') }}};
   {{{ makeSetValue('height', '0', 'rect.height', 'double') }}};
#endif

is very easy to do and very easy to maintain, but writing tests for both parts is very hard and time consuming to do and maintain, and adds to the test suite run time. That is why I recommend to avoid these kinds of combinatorial testing, and instead focus on just testing the defaults. If someone finds a bug in a specific combination of the old paths, then we can add testing reactively.

sbc100 · 2019-12-04T17:18:40Z

Ok so we will continue to do most of our testing with only default MIN_VERSIONs and occasionally add specific tests as need for older combinations.

That sounds feasible for us, but a little sad for our users who will be running code that the CI never runs. But to be fair we don't run out tests against older browsers either so...

We might want to warn people when stray from the tested path: "You chose a non-default, and possibly less-tested browser version combo", if not at compile time at least in the documentation?

kripken · 2019-12-04T21:24:58Z

@juj I'm confused about your strong objection to LEGACY_VM_SUPPORT - all I am proposing that it be, is to be defined in settings.js, and a few lines in emcc.py that flip the MIN_X_VERSION flags to appropriate values. That's it - it would have no code complexity burden almost, and is just useful for backwards compatibility + for users that don't know exactly which old browsers they want to support, but want to support as many as possible (consider a 1-person team that is pretty new to the Web).

cc @Brion for thoughts on LEGACY_VM_SUPPORT

juj · 2019-12-05T09:13:32Z

all I am proposing that it be, is to be defined in settings.js, and a few lines in emcc.py that flip the MIN_X_VERSION flags to appropriate values.

Ah, yeah, that sounds good to me.

juj · 2019-12-06T07:34:09Z

Rebased this on top of incoming, good for re-review.

kainino0x · 2019-12-06T23:23:28Z

src/library_webgl.js

+        // If not chrome, fall through to return undefined. (undefined <= integer will yield false)
+      }
+      if (getChromeVersion() <= 57) {
+        WebGL2RenderingContext = undefined;


How does hiding this prototype definition actually cause Emscripten to disable WebGL 2? (Maybe a comment to explain why it works)

That said, I would prefer if we didn't modify the page global state. Is there a way we can disable Emscripten's use of WebGL 2 without doing that?

Good points both. Updated to avoid modifying global page state.

…urden on code size. Reduces -15.9% of code size (997 bytes) in https://github.com/juj/webgl_render_test for WebGL 2. Introduce new setting -s OLDEST_SUPPORTED_CHROME_VERSION=x to enable customizing which Chrome versions to target.

…o MIN_CHROME_VERSION

…points

src/library_webgl.js

* Remove support for WebGL 2 in Chrome 57 due to its high maintenance burden on code size. Reduces -15.9% of code size (997 bytes) in https://github.com/juj/webgl_render_test for WebGL 2. Introduce new setting -s OLDEST_SUPPORTED_CHROME_VERSION=x to enable customizing which Chrome versions to target. * Update other.test_minimal_runtime_code_size * Fix chrome version test, and rename OLDEST_SUPPORTED_CHROME_VERSION to MIN_CHROME_VERSION * Update comment on old Chrome 57 not supporting WebGL 2 GC free entry points * Remove duplicate MIN_CHROME_VERSION

gkv311 · 2021-02-14T14:26:17Z

Just two cents - Safari 14 (which is still the latest stable release) has an "Experimental WebGL 2.0" option which triggers the same scenario as removed context.supportsWebGL2EntryPoints - it fails on calling functions like glUniform4fv() using garbage-free WebGL 2.0 entry points.

It's a pity that there is no more place to put a workaround for this broken Safari... (hopefully this bug will be fixed in closest Safari updates).

#if USE_WEBGL2
      // BUG: Workaround Chrome WebGL 2 issue: the first shipped versions of WebGL 2 in Chrome did not actually implement the new WebGL 2 functions.
      //      Those are supported only in Chrome 58 and newer.
      function getChromeVersion() {
        var raw = navigator.userAgent.match(/Chrom(e|ium)\/([0-9]+)\./);
        return raw ? parseInt(raw[2], 10) : false;
      }
      context.supportsWebGL2EntryPoints = (context.version >= 2) && (getChromeVersion() === false || getChromeVersion() >= 58);
#endif

juj · 2021-02-14T14:32:42Z

@gkv311 That issue should be fixed on Safari Tech Preview, I remember having a video call with their engineers about this issue some months ago. So when their Experimental WebGL 2 reaches stable release, they should be working properly on this front.

This test is designed to verify the minimum code size so I can't see why we would want to enable support for an older browser here (potentially triggering the inclusion of polyfills). This was added #9919 but its not clear to me why.

…FC (#19793) This test is designed to verify the minimum code size so I can't see why we would want to enable support for an older browser here (potentially triggering the inclusion of polyfills). This was added #9919 but its not clear to me why.

juj added the GL label Nov 28, 2019

juj force-pushed the remove_supports_webgl2_entry_points branch from 375f5f1 to 3194635 Compare November 28, 2019 09:56

juj force-pushed the remove_supports_webgl2_entry_points branch 2 times, most recently from 7c5c477 to a6a0686 Compare November 28, 2019 10:47

juj force-pushed the remove_supports_webgl2_entry_points branch from a6a0686 to ebcf568 Compare November 29, 2019 15:44

juj force-pushed the remove_supports_webgl2_entry_points branch from ebcf568 to f371b63 Compare December 3, 2019 10:34

kripken reviewed Dec 3, 2019

View reviewed changes

kainino0x reviewed Dec 3, 2019

View reviewed changes

juj force-pushed the remove_supports_webgl2_entry_points branch from f371b63 to b6c44ae Compare December 4, 2019 09:30

juj force-pushed the remove_supports_webgl2_entry_points branch from ed2b023 to 3682d49 Compare December 6, 2019 07:31

kainino0x reviewed Dec 6, 2019

View reviewed changes

juj added 4 commits December 7, 2019 12:29

Update other.test_minimal_runtime_code_size

4a8da41

Fix chrome version test, and rename OLDEST_SUPPORTED_CHROME_VERSION t…

a806ea5

…o MIN_CHROME_VERSION

Update comment on old Chrome 57 not supporting WebGL 2 GC free entry …

e44171c

…points

juj force-pushed the remove_supports_webgl2_entry_points branch from 1de4c28 to 5b7bc11 Compare December 7, 2019 10:30

Remove duplicate MIN_CHROME_VERSION

1a7ba44

juj force-pushed the remove_supports_webgl2_entry_points branch from 5b7bc11 to 1a7ba44 Compare December 7, 2019 13:36

kainino0x reviewed Dec 9, 2019

View reviewed changes

src/library_webgl.js Show resolved Hide resolved

kainino0x approved these changes Dec 9, 2019

View reviewed changes

juj merged commit 3f05b18 into emscripten-core:incoming Dec 9, 2019

sbc100 mentioned this pull request Jul 6, 2023

Remove -sMIN_CHROME_VERSION=58 from test_minimal_runtime_code_size. NFC #19793

Merged

Remove supportsWebGL2EntryPoints #9919

Remove supportsWebGL2EntryPoints #9919

Conversation

juj commented Nov 28, 2019 • edited Loading

juj commented Nov 28, 2019

juj commented Nov 28, 2019

VirtualTim commented Nov 29, 2019

juj commented Dec 2, 2019

juj commented Dec 2, 2019

juj commented Dec 2, 2019

sbc100 commented Dec 2, 2019

juj commented Dec 2, 2019 • edited Loading

kripken commented Dec 3, 2019

Choose a reason for hiding this comment

juj Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Dec 4, 2019

kainino0x commented Dec 4, 2019

sbc100 commented Dec 4, 2019

kainino0x commented Dec 4, 2019

VirtualTim commented Dec 4, 2019

juj commented Dec 4, 2019

sbc100 commented Dec 4, 2019

kripken commented Dec 4, 2019

juj commented Dec 5, 2019

juj commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkv311 commented Feb 14, 2021

juj commented Feb 14, 2021

juj commented Nov 28, 2019 •

edited

Loading

juj commented Dec 2, 2019 •

edited

Loading

juj Dec 3, 2019 •

edited

Loading