dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.3k stars 4.48k forks source link

[browser] load core assemblies first #100141

Closed pavelsavara closed 1 week ago

pavelsavara commented 2 months ago

start MonoVM when essential DLLs are downloaded and continue downloading the rest of the assets

I estimate that this could shave 50ms from cold start.

other changes

Resurrection of https://github.com/dotnet/runtime/pull/93480

dotnet-policy-service[bot] commented 2 months ago

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

lambdageek commented 2 months ago

@kg this is close to what you've been thinking about recently

kg commented 2 months ago

thanks for doing the hard work on this! i agree on your estimated startup reduction, should be a win

pavelsavara commented 2 months ago

we may need runtimeconfig.bin to be loaded into VFS before we start mono. And I'm not sure about ICU data.

kg commented 2 months ago

we may need runtimeconfig.bin to be loaded into VFS before we start mono. And I'm not sure about ICU data.

ICU data will only be needed if any runtime startup loads it. JS interop does with an accidental string operation right now. We could probably make it assert if there's an attempt to load ICU during this warming phase

lewing commented 2 months ago

I'm not sure if my recollection is correct but I think we explicitly load the icu data earlier than we need to as well. It might be worth exploring if we can delay it longer.

pavelsavara commented 2 months ago

I'm not sure if my recollection is correct but I think we explicitly load the icu data earlier than we need to as well. It might be worth exploring if we can delay it longer.

This currently postpones all but core DLLs

pavelsavara commented 2 months ago

/azp run runtime-wasm

azure-pipelines[bot] commented 2 months ago
Azure Pipelines successfully started running 1 pipeline(s).
pavelsavara commented 2 months ago

Right now, this PR is not AOT friendly because AOT assumes that metadata would be available for all images.

pavelsavara commented 2 months ago

we need System.Threading.Channels to be part of the core for MT build. @maraf

pavelsavara commented 1 month ago

Current CI fails with

[20:31:58] fail: [0x0118c038-dpty 20:31:58.466] MONO_WASM: start_runtime() failed {}

There is mono_runtime_class_init_full on stack for most of them.

maraf commented 1 month ago

We need to land this after https://github.com/dotnet/sdk/pull/39689

pavelsavara commented 1 month ago

we need to make ICU data "core" when the globalizationMode not invariant

image

pavelsavara commented 1 month ago

we need System.Threading.Channels to be part of the core for MT build. @maraf

with the last change maybe not. I need to try without.

pavelsavara commented 1 month ago

hmm, Log

[13:54:11] fail: Error in mono_download_assets: RangeError: Start offset -1 is outside the bounds of the buffer
[13:54:11] fail: [--000eafbc-emsc 13:54:11.333] MONO_WASM: Start offset -1 is outside the bounds of the buffer
                 RangeError: Start offset -1 is outside the bounds of the buffer
                     at new Uint8Array (<anonymous>)
                     at http://127.0.0.1:41625/_framework/dotnet.runtime.js:3:77638
                     at Object.pa [as instantiate_asset] (http://127.0.0.1:41625/_framework/dotnet.runtime.js:3:77686)
                     at n (http://127.0.0.1:41625/_framework/dotnet.js:3:8629)
                     at async Promise.all (index 129)
[13:54:11] fail: [out of order message from the browser]: http://127.0.0.1:41625/_framework/dotnet.runtime.js 2:77637 Uncaught RangeError: Start offset -1 is outside the bounds of the buffer
[13:54:11] info: WASM EXIT 1
pavelsavara commented 1 month ago

Above fail is ICU asset data being loaded into memory via mono_wasm_load_bytes_into_heap_persistent on MT build in UI thread, while the mono is starting on deputy thread.

It seems that Module._sbrk returned -1 🤯

@kg @lewing ideas ?

See also https://github.com/emscripten-core/emscripten/pull/20793/files

kg commented 1 month ago

Maybe we should just retry if it returns -1 in MT builds? We may have just lost some sort of race. I'm surprised this is possible though, and it would suggest that malloc can fail if we're unlucky too.

pavelsavara commented 1 month ago

It's kind of unrelated to this PR I moved the discussion to https://github.com/dotnet/runtime/issues/96546#issuecomment-2065933037

pavelsavara commented 1 month ago

@maraf this is now unblocked, right ?

maraf commented 1 month ago

@maraf this is now unblocked, right ?

Yes, once https://github.com/dotnet/sdk/pull/39689 flows through installer and we get updated installer dependency for WBT

maraf commented 2 weeks ago
  • make assets relative to / when not absolute

Related https://github.com/dotnet/runtime/issues/99539

maraf commented 2 weeks ago

This is now unblocked in SDK

pavelsavara commented 2 weeks ago

I filled https://github.com/dotnet/runtime/issues/102196 and https://github.com/dotnet/runtime/issues/102195

pavelsavara commented 1 week ago

I filled https://github.com/dotnet/runtime/issues/102251

pavelsavara commented 1 week ago

/ba-g all issues are known

pavelsavara commented 1 week ago

60ms for firefox and 30ms for chrome during the cold start. image