Skip to content

Skip all validation when 'no-validation' pass is specified #15128

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andrewevstyukhin
Copy link
Contributor

Discussion in WebAssembly/binaryen#4167

Before:

[PassRunner] running passes
[PassRunner] running pass: generate-i64-dyncalls... 0.153705 seconds.
[PassRunner] (validating)
[PassRunner] running pass: legalize-js-interface... 0.0618519 seconds.
[PassRunner] (validating)
[PassRunner] running pass: strip-target-features... 0.022499 seconds.
[PassRunner] (validating)
[PassRunner] passes took 0.238055 seconds.
[PassRunner] (final validation)

After:

[PassRunner] running passes
[PassRunner] running pass: generate-i64-dyncalls... 0.167555 seconds.
[PassRunner] running pass: legalize-js-interface... 0.0663762 seconds.
[PassRunner] running pass: strip-target-features... 0.0121898 seconds.
[PassRunner] passes took 0.246121 seconds.

Adds test other.test_binaryen_passes_no_validation

extras = settings.BINARYEN_EXTRA_PASSES.split(',')
# we support both '-'-prefixed and unprefixed pass names
if '--no-validation' in extras or 'no-validation' in extras:
cmd += ['--no-validation']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a the right place to put --no-validation.. its BINARYEN_EXTRA_PASSES is, up until now, something the only effects the running of wasm-opt.

Perhaps a new -s NO_VALIDATE setting would make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, BINARYEN_EXTRA_PASSES is a single possible place to use --no-validation.

Default settings require my action:

struct PassOptions {
...
// Whether to run the validator to check for errors.
bool validate = true;

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that we already have BINARYEN_FEATURES which is almost what we want here, except that it is an internal setting atm. If we moved that from src/settings_internal.js to src/settings.js then this would work.

The name is because it is used to pass around features primarily, but nothing prevents other things from being there like --no-validation. Perhaps we should rename it though to BINARYEN_FLAGS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've be OK with adding BINARYEN_FLAGS assuming we can show this is really needed. I think in order to make it work while we would need to show a significant amount of the link time could be saved by adding --no-validation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(My preferred option would be will be to modify BINARYEN_PASS_DEBUG to have a mode that didn't slow things down quite as much since that is that what is specifically needed here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope to finish tests for next weekends. Then things can come clear.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 25, 2021

Can you explain why you don't want validation ? Is the module you are creating actual invalid from a binaryen POV?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 25, 2021

Ah.. I just read some more of the discussion in WebAssembly/binaryen#4167,

It sounds like the issue is with the way BINARYEN_PASS_DEBUG works. You want a version of BINARYEN_PASS_DEBUG that doesn't include extra validation. I would instead suggest a new option for BINARYEN_PASS_DEBUG. How about BINARYEN_PASS_DEBUG=fast or BINARYEN_PASS_DEBUG=summary .. WDYT?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 25, 2021

Ah.. I just read some more of the discussion in WebAssembly/binaryen#4167,

It sounds like the issue is with the way BINARYEN_PASS_DEBUG works. You want a version of BINARYEN_PASS_DEBUG that doesn't include extra validation. I would instead suggest a new option for BINARYEN_PASS_DEBUG. How about BINARYEN_PASS_DEBUG=fast or BINARYEN_PASS_DEBUG=summary .. WDYT?

Will follow up on the binaryen issue.

@andrewevstyukhin
Copy link
Contributor Author

andrewevstyukhin commented Sep 26, 2021

Can you explain why you don't want validation ? Is the module you are creating actual invalid from a binaryen POV?

I just need better logging of current action to guess a cause of out of memory crash on CI.
My wasm is very big and validation eats minutes in cloud.

In #14174 I showed that validation is times longer than most real passes.

@andrewevstyukhin
Copy link
Contributor Author

Will follow up on the binaryen issue.

imho CI has some bugs also. For example, on Windows machine I can't link:

c:\repo>em++ tests\hello_world.cpp
wasm-ld: error: cannot open output file C:\TEMP\tmp317_1gs3.wasm: function not supported

while

c:\repo\tests>em++ hello_world.cpp

works fine for current directory.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 26, 2021

Will follow up on the binaryen issue.

imho CI has some bugs also. For example, on Windows machine I can't link:

c:\repo>em++ tests\hello_world.cpp
wasm-ld: error: cannot open output file C:\TEMP\tmp317_1gs3.wasm: function not supported

while

c:\repo\tests>em++ hello_world.cpp

works fine for current directory.

What CI system are you refering too? What kind of windows machines / virtualization environment does it use? The error you are seeing looks like a very low level error from llvm trying to map a file into memory.

Can you explain why you don't want validation ? Is the module you are creating actual invalid from a binaryen POV?

I just need better logging of current action to guess a cause of out of memory crash on CI.
My wasm is very big and validation eats minutes in cloud.

In #14174 I showed that validation is times longer then most real passes.

In that issue @kripken mentions that validation is only costly when BINARYEN_PASS_DEBUG is set. He says:
"if you remove that env var, the time spent in validation should be barely noticeable". Are you saying that you are seeing validation being expensive even without BINARYEN_PASS_DEBUG?

@andrewevstyukhin
Copy link
Contributor Author

Without BINARYEN_PASS_DEBUG I can't say anything about timings because log lacks such information.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 27, 2021

BTW we do have tool called emprofile which will measure the cost of each step in the compilation and linking process and create nice html visualization for you. It isn't able to dig into what part of binaryen take time but it could be very usefull or profiling in general. To use it you would run you link command with EMPROFILE=1 .. more information here: https://emscripten.org/docs/optimizing/Profiling-Toolchain.html

@andrewevstyukhin
Copy link
Contributor Author

Now I use magic set temp=<hdd>:\temp on Windows 10 machine and can debug tests:

tests\runner other.test_binaryen_passes_no_validation

  1. What an interesting thing - output to stderr, not to stdout and so empty out = build().
  2. wasm-metadce seems to not accept --no-validation option:

[PassRunner] running passes
[PassRunner] running pass: remove-unused-module-elements... 5.29e-05 seconds.
[PassRunner] (validating)
[PassRunner] running pass: reorder-functions... 0.0001305 seconds.
[PassRunner] (validating)
[PassRunner] passes took 0.0001834 seconds.
[PassRunner] (final validation)

-g3 can avoid metadce in test.

Ok, I passed all tests ✔️

@stale
Copy link

stale bot commented Nov 2, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants