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

JSPI - JSPI_<EXPORTS/IMPORTS> setting and docs update. #21932

Merged
merged 1 commit into from
May 14, 2024

Conversation

brendandahl
Copy link
Collaborator

  • Replace ASYNCIFY_IMPORTS and ASYNCIFY_EXPORTS with JSPI_IMPORTS and JSPI_EXPORTS.
  • Better document JSPI support.

@@ -3204,7 +3204,7 @@ def test_embind_return_value_policy(self):

def test_jspi_wildcard(self):
self.require_jspi()
self.emcc_args += ['-sASYNCIFY_EXPORTS=async*']
self.emcc_args += ['-sJSPI_EXPORTS=async*']
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 the only place we use this in the whole test suite?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are three others. I've left them though so we still test that option until we remove it. I suppose I could just leave one, if that's preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, for deprecated settings I'd prefer that all tests use the new thing.. and we have a single test for the deprecated one (can be version of an existing test).

@sbc100
Copy link
Collaborator

sbc100 commented May 14, 2024

Can you add settings that are actually deprecated to DEPRECATED_SETTINGS in settings.py?

- Replace `ASYNCIFY_IMPORTS` and `ASYNCIFY_EXPORTS` with
`JSPI_IMPORTS` and `JSPI_EXPORTS`.
- Better document JSPI support.
@brendandahl brendandahl enabled auto-merge (squash) May 14, 2024 22:38
@brendandahl brendandahl merged commit 2bc5e31 into emscripten-core:main May 14, 2024
29 checks passed
what exports will become async based on what could potentially call an
an async import (``ASYNCIFY_IMPORTS``). However, with JSPI, the async imports
and exports must be explicitly set using ``JSPI_IMPORTS`` and ``JSPI_EXPORTS``
settings.
Copy link
Member

Choose a reason for hiding this comment

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

Is it not possible to compute exports in the JSPI case so that users don't need to specify them explicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible and we started down that path in this PR. I think the big difference with JSPI, is all "JSPI'ed" exports return a promise even if they don't ever suspend. If we do the auto approach that could lead to exports returning a promise where it's not expected. I think this worked okay with asyncify since often the export didn't actually suspend, so the library code worked as expected.

We could do some hybrid approach, where we check to see what exports we think will suspend and if they're not in the list error out or give the user a list of suspending exports.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks, makes sense.

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

3 participants