Skip to content
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

bogus warning 'Output contains some very large functions' #20811

Closed
manxorist opened this issue Nov 30, 2023 · 6 comments · Fixed by #20812
Closed

bogus warning 'Output contains some very large functions' #20811

manxorist opened this issue Nov 30, 2023 · 6 comments · Fixed by #20812

Comments

@manxorist
Copy link
Contributor

Receiving the warning warning: Output contains some very large functions (5116 lines in vY), consider building source files with -Os or -Oz) is rather pointless when I am already compiling with -Oz. If emscripten thinks functions are too big (for whatever reason), it is its job to make them smaller instead of complaining to the user without providing any way to suppress the warning.
Big functions are fine - they do exist. Emscripten should frankly just shut up in this case.

@manxorist
Copy link
Contributor Author

This may be a result of using -g1 (see #20810), but the warning with default -g0 looks so scary that I am not willing to risk using it.

Still, the combination of these 2 issues makes it impossible to build libopenmpt without any warning with emscripten, and there is absolutely nothing in libopenmpt that I can do to avoid it. Emscripten currently fails our CI, and I am considering blacklisting compiling with 3.1.50.

@manxorist
Copy link
Contributor Author

Building with -g2 revealed that emscripten is complaining about https://github.com/OpenMPT/openmpt/blob/c79066216ca280341a09fccec1f7642a1edb8298/soundlib/Load_it.cpp#L411-L1359 and https://github.com/OpenMPT/openmpt/blob/c79066216ca280341a09fccec1f7642a1edb8298/src/mpt/io/tests/tests_io.hpp#L36-L562. While these functions are large, there is really nothing wrong with them at all, and it is very much not a compilers job to have opinions about the size of functions and complain with a ***WARNING*** to the user about it.

manxorist added a commit to manxorist/emscripten that referenced this issue Nov 30, 2023
4 years ago, commit 49d7717 removed
OUTLINING_LIMIT which was the only actionable item a user could do to avoid
this warning. Suggesting to compile with -Oz when the user is already
compiling with -Oz is just supidly insulting and not contructive user
feedback.

It is not Emscripten's job to have opinions about the size of functions.

Fixes <emscripten-core#20811>.
manxorist added a commit to manxorist/emscripten that referenced this issue Nov 30, 2023
4 years ago, commit 49d7717 removed
OUTLINING_LIMIT which was the only actionable item a user could do to avoid
this warning. Suggesting to compile with -Oz when the user is already
compiling with -Oz is just stupidly insulting and not contructive user
feedback.

It is not Emscripten's job to have opinions about the size of functions.

Fixes <emscripten-core#20811>.
manxorist added a commit to manxorist/emscripten that referenced this issue Nov 30, 2023
4 years ago, commit 49d7717 removed
OUTLINING_LIMIT which was the only actionable item a user could do to avoid
this warning. Suggesting to compile with -Oz when the user is already
compiling with -Oz is just stupidly insulting and not constructive user
feedback.

It is not Emscripten's job to have opinions about the size of functions.

Fixes <emscripten-core#20811>.
@sbc100
Copy link
Collaborator

sbc100 commented Nov 30, 2023

Quick question: Are you seeing this warning when building with -sWASM=0 (i.e. wasm2js)?

@kripken do you know of any reason why such large JS function might be problem for wasm2js users these days?

@manxorist
Copy link
Contributor Author

Quick question: Are you seeing this warning when building with -sWASM=0 (i.e. wasm2js)?

I get this warning with -s WASM=2 and with -s WASM=0, but not with -s WASM=1.

@kripken do you know of any reason why such large JS function might be problem for wasm2js users these days?

Even if this causes some kind of obscure performance problem for some JS engine, a warning that the user cannot do anything about must not exist.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 30, 2023

Even if this causes some kind of obscure performance problem for some JS engine, a warning that the user cannot do anything about must not exist.

Fair enough, I'm just trying to gather all the background information on this warning.

@kripken
Copy link
Member

kripken commented Nov 30, 2023

I think historically this was a major performance problem, but likely not today, and also far less important given wasm, anyhow. I agree this can be removed.

kripken pushed a commit that referenced this issue Nov 30, 2023
Users have little they can do if they see this warning, and we were showing it
even if the user built in the modes that the message suggest they use. in
addition, this warning is likely not important these days given the huge
improvements in JS VMs over the years.

Fixes #20811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants