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

Warn when async wasm compilation is disabled #5497

Merged
merged 1 commit into from Aug 22, 2017
Merged

Warn when async wasm compilation is disabled #5497

merged 1 commit into from Aug 22, 2017

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 22, 2017

Aside from being slow, it can make code fail in chrome, so we should warn at compile time when user options force sync compilation.

logging.warning('BINARYEN_ASYNC_COMPILATION requested, but disabled since not in wasm-only mode')
logging.warning('BINARYEN_ASYNC_COMPILATION requested, but disabled because of user options. ' + warning)
elif 'BINARYEN_ASYNC_COMPILATION=0' not in settings_changes:
logging.warning('BINARYEN_ASYNC_COMPILATION disabled due to user options. ' + warning)
Copy link
Member

Choose a reason for hiding this comment

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

Can't the warning just say that it's because of the selected BINARYEN_METHOD? It looks like that's the only condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little more complicated, any time we are not in wasm-only mode then this happens. For example, if we use the asm.js optimizer (for something like safe-heap, emterpreter, outlining, or other such things built on the asm.js optimizer) then we are not in wasm-only mode.

(With some work perhaps those options could still use async compilation even without wasm-only mode, but I'm not sure how much. Also not sure how high a priority that should be.)

Copy link
Member

Choose a reason for hiding this comment

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

Hm, ok. but it would still be nice if we could come up with a warning a little more actionable than "play around with random settings and see if the warning goes away". I don't really have any ideas right now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would be better to be much more granular. But the link to the wiki page in the error here should help. And I think there will be churn here with options moving to support async compilation, so I'd rather not hardcode a list that could get out of date.

I could be wrong though and we'll see users say this isn't a good enough warning, we can iterate on this then.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I forgot about the wiki link. Yeah we should put a list there of things that can force async off. Then this warning text is fine.

@kripken kripken merged commit 26606be into incoming Aug 22, 2017
@kripken kripken deleted the warn-sync branch August 22, 2017 21:46
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 this pull request may close these issues.

None yet

2 participants