Skip to content

Enable MINIMAL_RUNTIME code size tests for Wasm backend #10116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 9, 2020

Conversation

juj
Copy link
Collaborator

@juj juj commented Dec 27, 2019

Wasm backend seems to be about +30% bigger compared to fastcomp backend, haven't really digged in to what causes the differences, and if it's a constant/one-off size diff or a linear one. That's something to investigate later, but this establishes a good baseline.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprising that the wasm backend is larger - it's almost always smaller in my testing. But there are differences in how some code is used from musl instead of asm.js library functions, which maybe is noticeable in a small enough program that happens to use them. Definitely worth investigating.

@juj juj force-pushed the minimal_runtime_code_size_tests branch 2 times, most recently from 7dff5e3 to 5d9911b Compare January 7, 2020 15:05
@juj
Copy link
Collaborator Author

juj commented Jan 8, 2020

Anyone knows what might be up with the mac CI bot? It fails other.test_minimal_runtime_code_size with

/usr/local/opt/python3/bin/python3.6 /Users/distiller/project/emcc.py -o a.html /Users/distiller/project/tests/minimal_webgl/main.cpp /Users/distiller/project/tests/minimal_webgl/webgl.c --js-library /Users/distiller/project/tests/minimal_webgl/library_js.js -s RUNTIME_FUNCS_TO_IMPORT=[] -s USES_DYNAMIC_ALLOC=2 -lGL -s MODULARIZE=1 -s MINIMAL_RUNTIME=2 -s AGGRESSIVE_VARIABLE_ELIMINATION=1 -s ENVIRONMENT=web -s TEXTDECODER=2 -s ABORTING_MALLOC=0 -s ALLOW_MEMORY_GROWTH=0 -s SUPPORT_ERRNO=0 -s DECLARE_ASM_MODULE_EXPORTS=1 -s MALLOC=emmalloc -s GL_EMULATE_GLES_VERSION_STRING_FORMAT=0 -s GL_EXTENSIONS_IN_PREFIXED_FORMAT=0 -s GL_SUPPORT_AUTOMATIC_ENABLE_EXTENSIONS=0 -s GL_TRACK_ERRORS=0 -s GL_SUPPORT_EXPLICIT_SWAP_CONTROL=0 -s GL_POOL_TEMP_BUFFERS=0 -s FAST_UNROLLED_MEMCPY_AND_MEMSET=0 -s MIN_CHROME_VERSION=58 --output_eol linux -O3 --closure 1 -DNDEBUG -ffast-math
error: undefined symbol: glAttachShader
warning: To disable errors for undefined symbols use `-s ERROR_ON_UNDEFINED_SYMBOLS=0`
error: undefined symbol: glBindAttribLocation
error: undefined symbol: glBindBuffer
error: undefined symbol: glBindTexture
error: undefined symbol: glBlendFunc
error: undefined symbol: glBufferData
error: undefined symbol: glClear
error: undefined symbol: glClearColor
error: undefined symbol: glCompileShader
error: undefined symbol: glCreateProgram
error: undefined symbol: glCreateShader
error: undefined symbol: glDrawArrays
error: undefined symbol: glEnable
error: undefined symbol: glEnableVertexAttribArray
error: undefined symbol: glGenBuffers
error: undefined symbol: glGenTextures
error: undefined symbol: glGetUniformLocation
error: undefined symbol: glLinkProgram
error: undefined symbol: glShaderSource
error: undefined symbol: glTexImage2D
error: undefined symbol: glTexParameteri
error: undefined symbol: glUniform4f
error: undefined symbol: glUniformMatrix4fv
error: undefined symbol: glUseProgram
error: undefined symbol: glVertexAttribPointer
Error: Aborting compilation due to previous errors
shared:ERROR: '/Users/distiller/emsdk-master/node/12.9.1_64bit/bin/node /Users/distiller/project/src/compiler.js /var/folders/ms/xg67k5sn16xc7sdr_w3q45840000gn/T/tmp214qulfv/tmpgqbgni7e.txt' failed (1)
test_minimal_runtime_code_size (test_other.other) ... ERROR

That is, it refuses to link in the JS WebGL library, even though it is building in non-strict mode where the WebGL libraries should come in by default; and even then it is explicitly passing -lGL into the build..

When I locally run the test on my Windows, Linux and macOS boxes, the test passes.

@kripken
Copy link
Member

kripken commented Jan 8, 2020

I'm not sure why it fails only on mac, but minimal runtime does disable AUTO_JS_LIBRARIES, so explicit -l*.js are needed, I think?

emscripten/emcc.py

Lines 1538 to 1551 in 58c147b

if shared.Settings.MINIMAL_RUNTIME:
# Minimal runtime uses a different default shell file
if options.shell_path == shared.path_from_root('src', 'shell.html'):
options.shell_path = shared.path_from_root('src', 'shell_minimal_runtime.html')
if shared.Settings.ASSERTIONS and shared.Settings.MINIMAL_RUNTIME:
# In ASSERTIONS-builds, functions UTF8ArrayToString() and stringToUTF8Array() (which are not JS library functions), both
# use warnOnce(), which in MINIMAL_RUNTIME is a JS library function, so explicitly have to mark dependency to warnOnce()
# in that case. If string functions are turned to library functions in the future, then JS dependency tracking can be
# used and this special directive can be dropped.
shared.Settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['$warnOnce']
# Require explicit -lfoo.js flags to link with JS libraries.
shared.Settings.AUTO_JS_LIBRARIES = 0

So somehow the emcc.py code in process_libraries, which should add library_webgl.js from -lGL, is not working on that machine..?

@juj juj force-pushed the minimal_runtime_code_size_tests branch 3 times, most recently from 182d81f to afa7e22 Compare January 8, 2020 19:12
@juj juj force-pushed the minimal_runtime_code_size_tests branch from 41a9883 to f1124a9 Compare January 8, 2020 20:52
@juj juj merged commit 4e442df into emscripten-core:incoming Jan 9, 2020
@juj
Copy link
Collaborator Author

juj commented Jan 9, 2020

The issue was that -lGL picks up libGL.a instead of library_webgl.js. Marked down #10171, I don't have time to look at that now.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
…ore#10116)

* Enable MINIMAL_RUNTIME code size tests for Wasm backend

* Add in debugging for JS libraries. Remove unnecessary function maybeAddGLDep().

* Debug macOS box

* Restore mac CI and fix GL linkage

* Update code size in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants