-
Notifications
You must be signed in to change notification settings - Fork 537
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
Add support for multi-thread and/or SIMD WebAssembly #2620
Conversation
de89c99
to
fc04fc6
Compare
This PR is not super useful since net7 and net8 don't support any of this. Maybe net9 will. |
@jairbubbles does this PR help at all? |
</PropertyGroup> | ||
<ItemGroup Condition="'$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true' and '$(TargetFrameworkVersion)' != ''"> | ||
<ItemGroup Condition="'$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true' and '$(TargetFrameworkVersion)' != '' and '$(ShouldIncludeNativeSkiaSharp)' == 'True'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since NativeFileReference
format is consistent for any Mono WASM project, would it make sense to use another property here?
Imagine something like:
<PropertyGroup>
<ShouldIncludeNativeSkiaSharp Condition="'$(ShouldIncludeNativeSkiaSharp)' == '' AND '$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true'">true</ShouldIncludeNativeSkiaSharp>
</PropertyGroup>
This way third party, like Avalonia, can set ShouldIncludeNativeSkiaSharp
to true.
And it will still support Blazor as a first party by defaulting to UsingMicrosoftNETSdkBlazorWebAssembly
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor note, Native AOT uses <NativeLibrary />
instead of <NativeFileReference/>
. But that's likely wouldn't be a concern here until .NET 10 or 11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeromelaban you folks have a different way of loading native things? You just read the @(Content)
and figure it out. Are there some checks that if we add will cause issues?
@maxkatz6 are you just the normal logic and no special checks? So you are suggesting that we set it up such that ShouldIncludeNativeSkiaSharp =True
always includes the bits without doing fancy checks, and leave the blazor check for rather the default value of ShouldIncludeNativeSkiaSharp
?
And for Uno, this means that it will not be set by default but we use the $(IsUnoHead)
value to add the Uno way.
@maxkatz6 is there a Is Avalonia App
property that I can check to always include? Or would you rather it be opt in? Or even opt out by setting it to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeromelaban you folks have a different way of loading native things? You just read the @(Content) and figure it out. Are there some checks that if we add will cause issues?
Indeed, this predates the support from the runtime, but it also supports automatic selection based on the path's format. Starting from net9, we'll transitively include the NativeFileReference
support.
And for Uno, this means that it will not be set by default but we use the $(IsUnoHead) value to add the Uno way.
We could also add support that property, but in general, the fact that '$(OutputType)' == 'exe'
may be enough to conditionally include assets. We had that $(IsUnoHead)
property in the past because some targets were requiring the head to be a dll
, not an exe
.
As for the condition suggested by @maxkatz6, I'd adjust it as:
<PropertyGroup>
<ShouldIncludeNativeSkiaSharp Condition="'$(ShouldIncludeNativeSkiaSharp)' == '' AND ( '$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true' OR '$(UsingMicrosoftNETSdkWebAssembly)' == 'true' )">true</ShouldIncludeNativeSkiaSharp>
</PropertyGroup>
to support the non-blazor Wasm SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxkatz6 is there a Is Avalonia App property that I can check to always include?
No, not really.
For our needs, there is a little of advantage of adding avalonia specific checks into SkiaSharp, since we completely reuse runtime NativeFileReference support. I.e. without any special handling.
The only exception of special handling was NativeAOT, but even then, it's the same build-in runtime feature (just a different runtime), not something Avalonia specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattleibow these changes look good to me.
The only thing, native files inclusion can't be forced, unless user uses correct SDK. As WASM projects can be used with a standard SDK just fine. But this change improves defaults anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say forced, are you saying I am forcing something here? Or are you saying it as a precaution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no.
I mean, some developer might want to enable ShouldIncludeNativeSkiaSharp, even if they don't use WASM SDK. Since it's possible to run Browser apps without WASM SDK. Most of older Avalonia Browser apps don't use WASM SDK for example.
It's not possible to force this property right now. But is possible to manually include these files, so that's not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By WASM SDK I mean <Project SDK="Microsoft.NET.Sdk.WebAssembly">
specifically. This PR includes a check for this (or Blazor) SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. The opposite of what I was thinking. You want to be able to force the native. Did you use the same item group name "NativeFileReference"?
@@ -14,15 +19,25 @@ | |||
</ItemGroup> | |||
|
|||
<!-- If this is a ASP.NET Blazor web assembly app --> | |||
<PropertyGroup Condition="'$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true'"> | |||
<PropertyGroup Condition="'$(UsingMicrosoftNETSdkBlazorWebAssembly)' == 'true' and '$(ShouldIncludeNativeSkiaSharp)' == 'True'"> | |||
<WasmBuildNative Condition="'$(WasmBuildNative)' == ''">true</WasmBuildNative> | |||
<EmccExtraLDFlags>$(EmccExtraLDFlags) -s USE_WEBGL2=1</EmccExtraLDFlags> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to me: I see Uno has -s OFFSCREEN_FRAMEBUFFER=1
in the flags, so I am wondering if we need that too for Blazor (I was not able to test before since this feature did not exist yet). However, this probably means it is actually needed so maybe I need to check that as well.
Not sure what I am doing wrong, but wasm and skia seem to be fighting https://github.com/mattleibow/SkiaSharpWasm logging.ts:43 MONO_WASM: instantiate_wasm_module() failed CompileError: WebAssembly.compileStreaming(): Compiling function #27054:"CompressionNative_Crc32" failed: expected 1 elements on the stack for fallthru, found 3 @+8047983 |
I've noticed something similar in another project, with net9 specifically. It's not related to threading nor to simd, for what I can tell. In my context, it is failing in debug but not in release. Is it the case for you as well? |
# Conflicts: # scripts/azure-templates-stages.yml
This appears to be the case for me too. I am trying to test the SIMD as well, but not sure how to "test" that. Is there a something that fails with SIMD disabled I can try to make sure I am doing it right? Setting |
The test I made was to use the non-SIMD version of skiasharp, based on the hints from @kg. Not sure why disabling SIMD entirely does have an effect, though. |
|
cc @radekdoulik |
When I run Avalonia with
No threading or simd. Works fine in Release, but not in Debug. |
Going to merge this now in an effort to get things working with .NET 9. If there are any changes with regards to MSBuild targets or some way we can do better, feel free to open a PR. Or you can open an issue. PRs are more cool though! |
Description of Change
Support multithreading or SIMD blazor apps out the box.
Bugs Fixed
API Changes
None.
Behavioral Changes
None.
Required skia PR
None.
PR Checklist