From 9b65e23a67c6130a28f848e42b6088820b7e4e59 Mon Sep 17 00:00:00 2001
From: Pavel Savara <pavel.savara@gmail.com>
Date: Fri, 5 May 2023 19:03:24 +0200
Subject: [PATCH] [browser] prevent <DebuggerSupport> from disabling interp
 optimizations (#85792)

* [wasm] Don't enable debugging for library tests by default

@pavelsavara and @brzvlad found that some tests like
System.Collections.Immutable.Tests run slower because the runtime has
debugging enabled which causes the interpreter to disable optimizations.

Reason:

- Some tests depend on debugger attributes, and thus set
  `$(DebuggerSupport)=true` to preserve them while linking.
- Which sets `$(WasmDebugLevel)=-1`. But setting `$(WasmDebugLevel)<0`
  enables debugging support in the runtime.

- So, set this to `<0` only when building with `Configuration=Debug` .

* set it to zero only on CI

---------

Co-authored-by: Ankit Jain <radical@gmail.com>
---
 eng/testing/tests.browser.targets | 11 ++++++++++-
 eng/testing/tests.wasi.targets    | 11 ++++++++++-
 eng/testing/tests.wasm.targets    | 14 ++++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/eng/testing/tests.browser.targets b/eng/testing/tests.browser.targets
index ac4796a..d358982 100644
--- a/eng/testing/tests.browser.targets
+++ b/eng/testing/tests.browser.targets
@@ -160,7 +160,16 @@
       <WasmGenerateRunV8Script>true</WasmGenerateRunV8Script>
 
       <WasmNativeDebugSymbols Condition="'$(DebuggerSupport)' == 'true' and '$(WasmNativeDebugSymbols)' == ''">true</WasmNativeDebugSymbols>
-      <WasmDebugLevel Condition="'$(DebuggerSupport)' == 'true' and '$(WasmDebugLevel)' == ''">-1</WasmDebugLevel>
+      <!--
+        Do this *after* importing WasmApp.targets. tests.wasm.targets sets this to `reset-to-zero` to indicate
+        that we want to force this value to zero.
+
+        WasmApp.targets *overrides* `WasmDebugLevel` when `DebuggerSupport=true`, but for the library tests
+        we explicitly want to:
+        1. build with DebuggerSupport=true so the debugger attributes are preserved by the linker;
+        2. *debugging* is disabled at run time so the interpreter optimizations don't get disabled.
+      -->
+      <WasmDebugLevel Condition="'$(WasmDebugLevel)' == 'reset-to-zero'">0</WasmDebugLevel>
     </PropertyGroup>
 
     <ItemGroup Condition="'$(IncludeSatelliteAssembliesInVFS)' == 'true' and '$(BuildAOTTestsOnHelix)' != 'true'">
diff --git a/eng/testing/tests.wasi.targets b/eng/testing/tests.wasi.targets
index 87dbe5f..14b217b 100644
--- a/eng/testing/tests.wasi.targets
+++ b/eng/testing/tests.wasi.targets
@@ -90,7 +90,16 @@
       <WasmInvariantGlobalization>$(InvariantGlobalization)</WasmInvariantGlobalization>
 
       <WasmNativeDebugSymbols Condition="'$(DebuggerSupport)' == 'true' and '$(WasmNativeDebugSymbols)' == ''">true</WasmNativeDebugSymbols>
-      <WasmDebugLevel Condition="'$(DebuggerSupport)' == 'true' and '$(WasmDebugLevel)' == ''">-1</WasmDebugLevel>
+      <!--
+        Do this *after* importing WasmApp.targets. tests.wasm.targets sets this to `reset-to-zero` to indicate
+        that we want to force this value to zero.
+
+        WasmApp.targets *overrides* `WasmDebugLevel` when `DebuggerSupport=true`, but for the library tests
+        we explicitly want to:
+        1. build with DebuggerSupport=true so the debugger attributes are preserved by the linker;
+        2. *debugging* is disabled at run time so the interpreter optimizations don't get disabled.
+      -->
+      <WasmDebugLevel Condition="'$(WasmDebugLevel)' == 'reset-to-zero'">0</WasmDebugLevel>
     </PropertyGroup>
 
     <ItemGroup Condition="'$(IncludeSatelliteAssembliesInVFS)' == 'true' and '$(BuildAOTTestsOnHelix)' != 'true'">
diff --git a/eng/testing/tests.wasm.targets b/eng/testing/tests.wasm.targets
index 9c01950..4d6abbf 100644
--- a/eng/testing/tests.wasm.targets
+++ b/eng/testing/tests.wasm.targets
@@ -6,6 +6,20 @@
     <ArchiveTests Condition="'$(WasmBuildingForNestedPublish)' == 'true'">false</ArchiveTests>
     <BundleTestAppTargets>$(BundleTestAppTargets);BundleTestWasmApp</BundleTestAppTargets>
     <DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' == 'Debug'">true</DebuggerSupport>
+    <!--
+        Some tests depend on debugger attributes, and thus set $(DebuggerSupport)=true to preserve
+        them while linking.
+        But setting WasmDebugLevel<0 disables optimizations in the interpreter. So setting that
+        based on $(DebuggerSupport) has unintended slow down.
+
+        But we do want to set it for Configuration=Debug .
+    -->
+    <WasmDebugLevel Condition="'$(Configuration)' == 'Debug' and '$(WasmDebugLevel)' == ''">-1</WasmDebugLevel>
+    <!-- We want to have WasmDebugLevel=0, but if DebuggerSupport=true then WasmApp.targets
+         *will* override it. So, set this to "reset-to-zero" here, and reset the value later
+         in targets. -->
+    <WasmDebugLevel Condition="'$(DebuggerSupport)' == 'true' and '$(ContinuousIntegrationBuild)' == 'true' and '$(WasmDebugLevel)' == ''">reset-to-zero</WasmDebugLevel>
+
     <TrimMode Condition="'$(TrimMode)' == ''">full</TrimMode>
 
     <!-- Some tests expect to load satellite assemblies by path, eg. System.Runtime.Loader.Tests,
-- 
2.7.4