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

[wasm] asset loading for workers #73484

Merged
merged 8 commits into from
Aug 9, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 5, 2022

  • move asset related logic to separate fileassets.ts
  • split logging code into separate file logging.ts
  • add dotnet.wasm and dotnet-crypto-worker.js to mono-config.json via WasmAppBuilder
  • moved readSymbolMapFile() call to startup.ts
  • fixed bug in setup_proxy_console() which didn't really copy the original log and error functions of the console and caused recursion on error.
  • moved init_crypto after mono_wasm_load_config in mono_wasm_pre_init_essential_async
  • added new resource type js-module-crypto and js-module-threads
  • changed the detection of blazor startup sequence to not be disabled by presence of config, but only by config.assets having some assembly in it.

Most of this changes were moved from #73073 where is log of discussions about it
Together with dotnet/aspnetcore#43049

- split logging code into separate file `logging.ts`
- add `dotnet.wasm` and `dotnet-crypto-worker.js` to mono-config.json
- moved `readSymbolMapFile()` call to `startup.ts`
- fixed bug in `setup_proxy_console()` which didn't really copy the original `log` and `error` functions of the console and caused recursion on error.
- moved `init_crypto` after `mono_wasm_load_config` in `mono_wasm_pre_init_essential_async`
- added new resource type `js-module-crypto` and `js-module-threads`
- changed the detection of blazor startup sequence to not be disabled by presence of config, but only by config.assets having some assembly in it.
@ghost
Copy link

ghost commented Aug 5, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • move asset related logic to separate fileassets.ts
  • split logging code into separate file logging.ts
  • add dotnet.wasm and dotnet-crypto-worker.js to mono-config.json via WasmAppBuilder
  • moved readSymbolMapFile() call to startup.ts
  • fixed bug in setup_proxy_console() which didn't really copy the original log and error functions of the console and caused recursion on error.
  • moved init_crypto after mono_wasm_load_config in mono_wasm_pre_init_essential_async
  • added new resource type js-module-crypto and js-module-threads
  • changed the detection of blazor startup sequence to not be disabled by presence of config, but only by config.assets having some assembly in it.
Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara requested a review from maraf August 5, 2022 18:35
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

LGTM

src/mono/wasm/runtime/startup.ts Outdated Show resolved Hide resolved
pavelsavara and others added 2 commits August 6, 2022 07:02
# Conflicts:
#	src/mono/wasm/runtime/crypto-worker.ts
#	src/mono/wasm/runtime/workers/dotnet-crypto-worker.js
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

Unfortunately the deadlock is still with us. Log

@radical
Copy link
Member

radical commented Aug 6, 2022

You can run the tests a good bunch(30?) of times on helix, which can be done in one shot as we had discussed offline. It is helpful in finding similar issues.

I wonder if we should always do that for tests that depend on workers, on CI.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Pull request contains merge conflicts.

# Conflicts:
#	src/mono/wasm/runtime/startup.ts
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

@radical
Copy link
Member

radical commented Aug 9, 2022

The wasm/debugger test failures are unrelated, and will be fixed by #73524 .

@radical
Copy link
Member

radical commented Aug 9, 2022

We should queue up the crypto deadlock fix right after this. If this happens to cause any issue with crypto, then it can be fixed in that PR.

@pavelsavara pavelsavara merged commit de03d8a into dotnet:main Aug 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2022
@pavelsavara pavelsavara deleted the wasm_load_crypto_asset branch November 29, 2022 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants