Skip to content

Speed up emcc -v by not running sanity checks. #13176

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 7, 2021
Merged

Speed up emcc -v by not running sanity checks. #13176

merged 1 commit into from
Jan 7, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 3, 2021

ince some build tools run emcc -v regularly we want to be fast. This
change avoid running sanity checks which means fewer subprocesses.

On my machine this takes emcc -v from ~300 ro ~200 ms.

Sanity checks are still run on first use.

See: #13004

@sbc100 sbc100 requested a review from kripken January 3, 2021 10:32
@kripken
Copy link
Member

kripken commented Jan 4, 2021

A downside of this is that then there is no easy way for users to run those checks. The checks can find problems, and we sometimes tell a user to run emcc -v to see if it complains.

Perhaps we should add a separate option for that, if we want to make emcc -v fast?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 5, 2021

We have EMCC_DEBUG=1 and --clear-cache that will both force sanity checks. Also, sanity checks will always be performed on first use of the emcc. I would rather avoid adding more command line options.

This change has two parts. I could revert that part of this change that running the checks when -v is used with other arguments? So then ./emcc -v would not run the checks but emcc -v -c hello.c would run the checks.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 5, 2021

(BTW I can't remember a time when I've found it useful that emcc -v force running on the sanity check).

kripken
kripken reviewed Jan 5, 2021
View reviewed changes
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.

I'd forgotten about EMCC_DEBUG=1 running those checks.

However, I see that our docs do use emcc -v for these checks (by grep, and I read some of the pages to confirm). So we'd need to update those too. sgtm with that + a changelog entry.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 6, 2021

Added changelog and updated docs.

kripken
kripken reviewed Jan 6, 2021
View reviewed changes
site/source/docs/getting_started/FAQ.rst Outdated
@@ -17,7 +17,7 @@ Why do I get multiple errors building basic code and the tests?

All the tests in the :ref:`Emscripten test suite <emscripten-test-suite>` are known to build and pass on our test infrastructure, so if you see failures locally it is likely that there is some problem with your environment. (Rarely, there may be temporary breakage, but never on a tagged release version.)

First call ``emcc -v``, which runs basic sanity checks and prints out useful environment information. If that doesn't help, follow the instructions in :ref:`verifying-the-emscripten-environment`.
First call ``emcc -v``, prints out useful environment information. If that doesn't help, follow the instructions in :ref:`verifying-the-emscripten-environment`.
Copy link
Member

Choose a reason for hiding this comment

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

Another key doc I think we should update is the one linked to here, which goes to a section titled "Sanity tests", in site/source/docs/building_from_source/verify_emscripten_environment.rst. That should not mention -v any more, and please add a mention of EMCC_DEBUG=1 in the env.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking of just leaving that sections as emcc -v .. since that will actually run the sanity checks the first time around.

I guess maybe I will add a new option because its kind of annoying to ask folks to set the environment variable here.

@sbc100 sbc100 force-pushed the speed_up_dash_v branch 2 times, most recently from 9e63595 to 264fac6 Compare January 6, 2021 18:30
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 6, 2021

Added a new --check option.

kripken
kripken approved these changes Jan 6, 2021
View reviewed changes
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.

--check is a very good idea I think.

lgtm with comments.

@sbc100
Speed up emcc -v by not running sanity checks.
74926e9 
Since some build tools run `emcc -v` regularly we want to be fast.  This
change avoid running sanity checks which means fewer subprocesses.

On my machine this takes `emcc -v ` from ~300 ro ~200 ms.

Add a new options `--check` which will force the running of sanity
checks.

See: #13004
@sbc100 sbc100 merged commit d0d12c7 into master Jan 7, 2021
@sbc100 sbc100 deleted the speed_up_dash_v branch January 7, 2021 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @kripken