Skip to content

Commit

Permalink
[browser] Trigger relink on EmccMaximumHeapSize change (#105027)
Browse files Browse the repository at this point in the history
* Edit test + trigger relink.

* Remove logging to speed up the test + decrease loop runs to prevent "Browser has been disconnected" error.

* Feedback - properties are not bool-only anymore.

* Fix: workload needed when heap size set.

---------

Co-authored-by: Larry Ewing <lewing@microsoft.com>
  • Loading branch information
2 people authored and directhex committed Jul 26, 2024
1 parent 7942b5b commit f6bca30
Show file tree
Hide file tree
Showing 11 changed files with 49 additions and 29 deletions.
2 changes: 1 addition & 1 deletion eng/testing/linker/project.csproj.template
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@

<Target Name="RemoveInvariantGlobalization" BeforeTargets="_SetWasmBuildNativeDefaults" Condition="'$(TargetArchitecture)' == 'wasm'">
<ItemGroup>
<_BoolPropertiesThatTriggerRelinking Remove="InvariantGlobalization" />
<_PropertiesThatTriggerRelinking Remove="InvariantGlobalization" />
</ItemGroup>
</Target>

Expand Down
6 changes: 4 additions & 2 deletions src/mono/browser/browser.proj
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<WasmSingleFileBundle Condition="'$(WasmSingleFileBundle)' == ''">false</WasmSingleFileBundle>
<WasmEnableSIMD Condition="'$(WasmEnableSIMD)' == ''">true</WasmEnableSIMD>
<WasmEnableExceptionHandling Condition="'$(WasmEnableExceptionHandling)' == ''">true</WasmEnableExceptionHandling>
<EmccMaximumHeapSize Condition="'$(EmccMaximumHeapSize)' == ''">2147483648</EmccMaximumHeapSize>
<WasmEnableJsInteropByValue Condition="'$(WasmEnableJsInteropByValue)' == '' and '$(WasmEnableThreads)' == 'true'">true</WasmEnableJsInteropByValue>
<WasmEnableJsInteropByValue Condition="'$(WasmEnableJsInteropByValue)' == ''">false</WasmEnableJsInteropByValue>
<FilterSystemTimeZones Condition="'$(FilterSystemTimeZones)' == ''">false</FilterSystemTimeZones>
Expand Down Expand Up @@ -334,13 +335,14 @@
"WasmOptConfigurationFlags": [@(WasmOptConfigurationFlags -> '%22%(Identity)%22', ',')],
"EmccDefaultExportedFunctions": [@(EmccExportedFunction -> '%22%(Identity)%22', ',')],
"EmccDefaultExportedRuntimeMethods": [@(EmccExportedRuntimeMethod -> '%22%(Identity)%22', ',')],
"BoolPropertiesThatTriggerRelinking": [
"PropertiesThatTriggerRelinking": [
{ "identity": "InvariantTimezone", "defaultValueInRuntimePack": "$(InvariantTimezone)" },
{ "identity": "InvariantGlobalization", "defaultValueInRuntimePack": "$(InvariantGlobalization)" },
{ "identity": "WasmNativeStrip", "defaultValueInRuntimePack": "$(WasmNativeStrip)" },
{ "identity": "WasmSingleFileBundle", "defaultValueInRuntimePack": "$(WasmSingleFileBundle)" },
{ "identity": "WasmEnableSIMD", "defaultValueInRuntimePack": "$(WasmEnableSIMD)" },
{ "identity": "WasmEnableExceptionHandling", "defaultValueInRuntimePack": "$(WasmEnableExceptionHandling)" }
{ "identity": "WasmEnableExceptionHandling", "defaultValueInRuntimePack": "$(WasmEnableExceptionHandling)" },
{ "identity": "EmccMaximumHeapSize", "defaultValueInRuntimePack": "$(EmccMaximumHeapSize)" }
]
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
'$(RunAOTCompilation)' == 'true' or
'$(WasmBuildNative)' == 'true' or
'$(WasmGenerateAppBundle)' == 'true' or
'$(_UsingBlazorOrWasmSdk)' != 'true'" >true</_WasmNativeWorkloadNeeded>
'$(_UsingBlazorOrWasmSdk)' != 'true' or
'$(EmccMaximumHeapSize)' != '' " >true</_WasmNativeWorkloadNeeded>

<UsingBrowserRuntimeWorkload Condition="'$(_BrowserWorkloadNotSupportedForTFM)' == 'true'">false</UsingBrowserRuntimeWorkload>
<UsingBrowserRuntimeWorkload Condition="'$(UsingBrowserRuntimeWorkload)' == '' and '$(_WasmNativeWorkloadNeeded)' == 'true'">true</UsingBrowserRuntimeWorkload>
Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasi/wasi.proj
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@
{
"items": {
"WasmOptConfigurationFlags": [@(WasmOptConfigurationFlags -> '%22%(Identity)%22', ',')],
"BoolPropertiesThatTriggerRelinking": [
"PropertiesThatTriggerRelinking": [
{ "identity": "InvariantTimezone", "defaultValueInRuntimePack": "$(InvariantTimezone)" },
{ "identity": "InvariantGlobalization", "defaultValueInRuntimePack": "$(InvariantGlobalization)" },
{ "identity": "WasmNativeStrip", "defaultValueInRuntimePack": "$(WasmNativeStrip)" },
Expand Down
7 changes: 7 additions & 0 deletions src/mono/wasm/Wasm.Build.Tests/Common/CommandResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ public CommandResult(ProcessStartInfo startInfo, int exitCode, string output)
public CommandResult EnsureSuccessful(string messagePrefix = "", bool suppressOutput = false)
=> EnsureExitCode(0, messagePrefix, suppressOutput);

public CommandResult EnsureFailed(string messagePrefix = "", bool suppressOutput = false)
{
if (ExitCode == 0)
throw new XunitException($"{messagePrefix} Expected non-zero exit code but got 0: {StartInfo.FileName} {StartInfo.Arguments}");
return this;
}

public CommandResult EnsureExitCode(int expectedExitCode = 0, string messagePrefix = "", bool suppressOutput = false)
{
if (ExitCode != expectedExitCode)
Expand Down
13 changes: 11 additions & 2 deletions src/mono/wasm/Wasm.Build.Tests/TestAppScenarios/AppTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,24 @@ protected void BuildProject(
string? binFrameworkDir = null,
RuntimeVariant runtimeType = RuntimeVariant.SingleThreaded,
bool assertAppBundle = true,
bool expectSuccess = true,
params string[] extraArgs)
{
(CommandResult result, _) = BlazorBuild(new BlazorBuildOptions(
Id: Id,
Config: configuration,
BinFrameworkDir: binFrameworkDir,
RuntimeType: runtimeType,
AssertAppBundle: assertAppBundle), extraArgs);
result.EnsureSuccessful();
AssertAppBundle: assertAppBundle,
ExpectSuccess: expectSuccess), extraArgs);
if (expectSuccess)
{
result.EnsureSuccessful();
}
else
{
result.EnsureFailed();
}
}

protected void PublishProject(
Expand Down
26 changes: 14 additions & 12 deletions src/mono/wasm/Wasm.Build.Tests/TestAppScenarios/MemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,22 @@ public MemoryTests(ITestOutputHelper output, SharedBuildPerTestClassFixture buil
{
}

[Theory]
[InlineData("Release", true)]
[InlineData("Release", false)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/105283")]
public async Task AllocateLargeHeapThenRepeatedlyInterop(string config, bool buildNative)
// ActiveIssue: https://github.com/dotnet/runtime/issues/104618
[Fact, TestCategory("no-workload")]
public async Task AllocateLargeHeapThenRepeatedlyInterop_NoWorkload() =>
await AllocateLargeHeapThenRepeatedlyInterop();

[Fact]
public async Task AllocateLargeHeapThenRepeatedlyInterop()
{
// native build triggers passing value form EmccMaximumHeapSize to MAXIMUM_MEMORY that is set in emscripten
// in non-native build EmccMaximumHeapSize does not have an effect, so the test will fail with "out of memory"
string config = "Release";
CopyTestAsset("WasmBasicTestApp", "MemoryTests", "App");
string extraArgs = $"-p:EmccMaximumHeapSize=4294901760 -p:WasmBuildNative={buildNative}";
BuildProject(config, assertAppBundle: false, extraArgs: extraArgs);
string extraArgs = BuildTestBase.IsUsingWorkloads ? "-p:EmccMaximumHeapSize=4294901760" : "-p:EmccMaximumHeapSize=4294901760";
BuildProject(config, assertAppBundle: false, extraArgs: extraArgs, expectSuccess: BuildTestBase.IsUsingWorkloads);

var result = await RunSdkStyleAppForBuild(new (Configuration: config, TestScenario: "AllocateLargeHeapThenInterop", ExpectedExitCode: buildNative ? 0 : 1));
if (!buildNative)
Assert.Contains(result.TestOutput, item => item.Contains("Exception System.OutOfMemoryException: Out of memory"));
if (BuildTestBase.IsUsingWorkloads)
{
await RunSdkStyleAppForBuild(new (Configuration: config, TestScenario: "AllocateLargeHeapThenInterop"));
}
}
}
12 changes: 6 additions & 6 deletions src/mono/wasm/build/WasmApp.Common.targets
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@
<WasmOptConfigurationFlags ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="false" Output="true" />
<EmccDefaultExportedFunctions ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="false" Output="true" />
<EmccDefaultExportedRuntimeMethods ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="false" Output="true" />
<BoolPropertiesThatTriggerRelinking ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="false" Output="true" />
<PropertiesThatTriggerRelinking ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="false" Output="true" />
</ParameterGroup>
</UsingTask>

Expand All @@ -405,7 +405,7 @@

<!-- shared by browser/wasi -->
<Output TaskParameter="WasmOptConfigurationFlags" ItemName="_DefaulWasmOptConfigurationFlags" />
<Output TaskParameter="BoolPropertiesThatTriggerRelinking" ItemName="_BoolPropertiesThatTriggerRelinking" />
<Output TaskParameter="PropertiesThatTriggerRelinking" ItemName="_PropertiesThatTriggerRelinking" />
</ReadWasmProps>

<CreateProperty Value="%(_EmccPropItems.Value)" Condition="%(_EmccPropItems.Identity) != ''">
Expand Down Expand Up @@ -511,15 +511,15 @@
Text="$(_ToolchainMissingErrorMessage) SDK is required for AOT'ing assemblies." />

<ItemGroup>
<_ChangedBoolPropertiesThatTriggerRelinking Include="%(_BoolPropertiesThatTriggerRelinking.Identity)" Condition="'$(%(_BoolPropertiesThatTriggerRelinking.Identity))' != '' and
'$(%(_BoolPropertiesThatTriggerRelinking.Identity))' != '%(_BoolPropertiesThatTriggerRelinking.DefaultValueInRuntimePack)'" />
<_ChangedPropertiesThatTriggerRelinking Include="%(_PropertiesThatTriggerRelinking.Identity)" Condition="'$(%(_PropertiesThatTriggerRelinking.Identity))' != '' and
'$(%(_PropertiesThatTriggerRelinking.Identity))' != '%(_PropertiesThatTriggerRelinking.DefaultValueInRuntimePack)'" />
</ItemGroup>
<PropertyGroup>
<_WasmBuildNativeRequired Condition="@(_ChangedBoolPropertiesThatTriggerRelinking->Count()) > 0">true</_WasmBuildNativeRequired>
<_WasmBuildNativeRequired Condition="@(_ChangedPropertiesThatTriggerRelinking->Count()) > 0">true</_WasmBuildNativeRequired>
</PropertyGroup>

<Error Condition="'$(WasmBuildNative)' == 'false' and '$(_WasmBuildNativeRequired)' == 'true'"
Text="WasmBuildNative is required because %(_ChangedBoolPropertiesThatTriggerRelinking.Identity)=$(%(_ChangedBoolPropertiesThatTriggerRelinking.Identity)), but WasmBuildNative is already set to 'false'." />
Text="WasmBuildNative is required because %(_ChangedPropertiesThatTriggerRelinking.Identity)=$(%(_ChangedPropertiesThatTriggerRelinking.Identity)), but WasmBuildNative is already set to 'false'." />

<PropertyGroup>
<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(_WasmBuildNativeRequired)' == 'true'">true</WasmBuildNative>
Expand Down
4 changes: 2 additions & 2 deletions src/mono/wasm/testassets/WasmBasicTestApp/App/MemoryTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
using System.Text;
using System.Runtime.InteropServices.JavaScript;

public partial class MemoryTest // ?test=AllocateLargeHeapThenInterop
public partial class MemoryTest
{
[JSImport("countChars", "main.js")]
internal static partial int CountChars(string testArray);
Expand Down Expand Up @@ -37,7 +37,7 @@ internal static void Run()
string randomString = GenerateRandomString(1000);
try
{
for (int i = 0; i < 10000; i++)
for (int i = 0; i < 1000; i++)
{
int count = CountChars(randomString);
if (count != randomString.Length)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@
</ItemGroup>

<ItemGroup>
<BlazorWebAssemblyLazyLoad Include="Json$(LazyAssemblyExtension)" />
<BlazorWebAssemblyLazyLoad Include="Json$(WasmAssemblyExtension)" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ function testOutput(msg) {

function countChars(str) {
const length = str.length;
testOutput(`JS received str of ${length} length`);
return length;
}

Expand Down

0 comments on commit f6bca30

Please sign in to comment.