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

FileSystem calls over Atomics.wait instead of service worker #87

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented May 16, 2024

In combination with jupyterlite/jupyterlite#1383

This PR provides an implementation of the Emscripten FileSystem that communicates using Atomics.wait and SharedArrayBuffer instead of the blocking HTTP requests to a service worker.

I open a PR here in jupyterlite/xeus for convenience, but part of the code here should probably go upstream in jupyterlite. Done in jupyterlite/jupyterlite#1383

How to test it:

  1. Build your jupyterlite site as usual.
  2. Serve it using the proper headers, so that shared array buffers can be used: npx static-handler --cors --coop --coep --corp ./

Implementation details

Instead of directly using SharedArrayBuffers and Atomic.waits (which would require defining a proper communication protocol), I decided to go with using https://github.com/WebReflection/coincident which provides high level APIs for making blocking communication between worker <-> main thread.

Coincident comes as a replacement for comlink which we used for wrapping the worker kernel communication.

@jtpio jtpio added the enhancement New feature or request label May 16, 2024
@martinRenou martinRenou marked this pull request as draft May 16, 2024 13:55
@martinRenou
Copy link
Member Author

I'll turn this into draft and leave it open for discussions. But I'll open the proper PR upstream in JupyterLite to introduce the needed APIs for this to work nicely.

@jtpio
Copy link
Member

jtpio commented May 16, 2024

Nice, thanks @martinRenou for looking into this!

src/index.ts Show resolved Hide resolved
src/drivefs.ts Outdated Show resolved Hide resolved
This makes sure to have a working setup with jupyterlite<0.4.0 when
using service worker
@martinRenou
Copy link
Member Author

This is all working nicely (except ipyleaflet of course, because CORS are disabled...)

Screenshot from 2024-06-04 15-25-22

@martinRenou martinRenou marked this pull request as ready for review June 4, 2024 13:26
@martinRenou martinRenou requested a review from jtpio June 4, 2024 13:26
@jtpio
Copy link
Member

jtpio commented Jun 4, 2024

We should probably document how to enable this. It could be here in the jupyterlite-xeus docs, or maybe in the main JupyterLite documentation directly, as we will probably want to support it in the Pyodide kernel too?

@martinRenou
Copy link
Member Author

We should definitely look at enabling this for jupyterlite-pyodide too :)

Agreed we should add it to the docs.

@jtpio
Copy link
Member

jtpio commented Jun 5, 2024

Testing locally: one advantage of this approach is that it works in a Firefox private window 👍

image

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Tested locally with and without the COOP / COEP headers to check it falls back to using the Service Worker 👍

Thanks for working on this!

@jtpio jtpio merged commit a80ff1b into jupyterlite:main Jun 5, 2024
12 checks passed
@martinRenou martinRenou deleted the shared_array_buffer_draft branch June 5, 2024 07:54
@jtpio
Copy link
Member

jtpio commented Jun 5, 2024

@martinRenou should this be released in a 0.1.9, or 0.2.0?

@martinRenou
Copy link
Member Author

martinRenou commented Jun 5, 2024

This should be backward compatible if not setting --cors --coop --coep --corp (actually the CI passed because it's backward compatible in that case, we're still using jupyterlite 0.3.x there).

If people were already setting --cors --coop --coep --corp, file access will eventually fail and crash the kernel due to https://github.com/jupyterlite/xeus/pull/87/files#diff-a0841713a169cff378e9935d6ae3ba34d57653c34fcea3294ac4771db5427b14R185

So I believe this is backward incompatible.

@martinRenou
Copy link
Member Author

I believe we can't make it backward compatible though, so we should probably make an alpha for 0.2.0. Would you like to do it ?

@jtpio
Copy link
Member

jtpio commented Jun 5, 2024

Yeah I was thinking a 0.2.0 would be on the safer side too.

Starting a 0.2.0-alpha.0 now.

@jtpio
Copy link
Member

jtpio commented Jun 6, 2024

We should probably document how to enable this. It could be here in the jupyterlite-xeus docs, or maybe in the main JupyterLite documentation directly,

Started to document this in jupyterlite/jupyterlite#1405

@martinRenou
Copy link
Member Author

I can have a look at implementing the shared array buffers approach for the pyodide kernel.

@jtpio
Copy link
Member

jtpio commented Jun 6, 2024

I can have a look at implementing the shared array buffers approach for the pyodide kernel.

That would be great, thanks! Hopefully the approach will be similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants