[wasm][debugger] Improve debugger performance based on JMC #86982

thaystg · 2023-05-31T22:05:23Z

If JMC is enabled and runtime doesn't have debug symbols of an assembly it means that the assembly being used is not from the customer, otherwise the pdb would be loaded by the runtime.
In this case we don't need to load any information about the assembly avoiding spending a lot of time receiving all the bytes from all the assemblies that will not be used because JMC is disabled.

UPDATE: I'm loading the information by demand, if we need any information from an assembly that is not loaded, like to inspect variables of a specific type that is not in the user code, I'm loading, it's a better approach to avoid any side effect and we are still spending less time because we will not need to load 149 assemblies at the startup even if we will not use them.

Measurement: From 4 seconds to load all the symbols during the app startup to less than 1 second.

I'm not adding any new test because this is already tested.

ghost · 2023-05-31T22:05:37Z

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

If JMC is enabled and runtime doesn't have debug symbols it means that the assembly being used is not from the customer, otherwise the pdb would be loaded by the runtime.
In this case we don't need to load any information about the assembly avoiding spending a lot of time receiving all the bytes from all the assemblies that will not be used because JMC is disabled.

Author:	thaystg
Assignees:	thaystg
Labels:	`area-Debugger-mono`
Milestone:	-

thaystg · 2023-05-31T22:07:14Z

@lewing do you think this makes sense?
If you agree I'll continue working on it. The customer can "DISABLE" just my code during the debugging session, then I'll need to load all the symbols from runtime at that moment.

ghost · 2023-05-31T22:07:31Z

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

Issue Details

If JMC is enabled and runtime doesn't have debug symbols it means that the assembly being used is not from the customer, otherwise the pdb would be loaded by the runtime.
In this case we don't need to load any information about the assembly avoiding spending a lot of time receiving all the bytes from all the assemblies that will not be used because JMC is disabled.
From 4 seconds to load all the symbols during the app startup to less than 1 second.

Author:	thaystg
Assignees:	thaystg
Labels:	`arch-wasm`, `area-Debugger-mono`
Milestone:	-

lewing · 2023-05-31T23:15:07Z

@lewing do you think this makes sense? If you agree I'll continue working on it. The customer can "DISABLE" just my code during the debugging session, then I'll need to load all the symbols from runtime at that moment.

yeah, I think it makes sense. There is no particular issue with loading them later once the switch changes right?

thaystg · 2023-06-01T19:07:39Z

@lewing do you think this makes sense? If you agree I'll continue working on it. The customer can "DISABLE" just my code during the debugging session, then I'll need to load all the symbols from runtime at that moment.

yeah, I think it makes sense. There is no particular issue with loading them later once the switch changes right?

No particular issue. When you disable JMC after the debugger session is started we will need to get the PDB from cache or from symbol server anyway. With this change we will also read the assembly bytes from runtime before try to get the symbols from cache / from symbol server.

…is started.

…t of time loading information from 149 assemblies if we will probably not need all of them.

src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

ilonatommy · 2023-06-20T07:33:06Z

src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs

+        internal System.Guid PdbGuid { get; set; }
+        internal bool IsPortableCodeView { get; set; }
+        internal string PdbName { get; set; }
+        internal bool CodeViewInformationAvailable { get; set; }


Do we ever make use of these setters? It does not look like a necessary change.

The property seems to be never read at all.

…sed_on_jmc

…mbly.

src/mono/mono/component/debugger-agent.c

ilonatommy

The idea with strrchr looks good as we use it in other places. Other than that, looks great.

radical · 2023-07-07T01:18:05Z

src/mono/mono/component/debugger-agent.c

+			break;
+		}
+	}
+


Maybe check if the string is empty here?

src/mono/mono/component/debugger-agent.c

radical · 2023-07-07T01:19:43Z

src/mono/mono/component/debugger-agent.c

+	MonoAssemblyByNameRequest byname_req;
+	mono_assembly_request_prepare_byname (&byname_req, mono_alc_get_default ());
+	MonoAssembly *assembly = mono_assembly_request_byname (aname, &byname_req, &status);
+	g_free (lookup_name);


Can we do this right after the call to mono_assembly_name_new?

radical · 2023-07-07T01:21:51Z

src/mono/mono/component/debugger-agent.c

+		g_ptr_array_free (assemblies, TRUE);
+		if (!assembly) {
+			PRINT_DEBUG_MSG (1, "Could not resolve assembly %s\n", assembly_name);
+			mono_assembly_name_free_internal (aname);


Should this jump to 6988, which can free and return assembly (==NULL here)?

src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs

radical · 2023-07-07T04:07:12Z

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

        {
+            if (!assemblyName.StartsWith("System.Private.CoreLib") && !ignoreJMC && proxy.JustMyCode && !(await HasDebugInfoLoadedByRuntime(assemblyName, token)))


StringComparison.Ordinal with StartsWith

radical · 2023-07-07T04:07:58Z

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

        {
+            if (!assemblyName.StartsWith("System.Private.CoreLib") && !ignoreJMC && proxy.JustMyCode && !(await HasDebugInfoLoadedByRuntime(assemblyName, token)))


Also, I would suggest maybe splitting this only multiple lines, and comments for why the different parts of the condition are needed.

radical · 2023-07-07T04:13:18Z

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

-            ret[1] = pdb_buf;
+            AssemblyAndPdbData ret = new(assembly_buf, pdb_buf);
+
+            if (MajorVersion == 2 && MinorVersion >= 64)


Should these checks be using MajorVersion >= 2?
Also, can we (in a followup) convert such version checks into properties named for whatever feature we want to check for. It would essentially document why we are doing the version check. And also, in future if we need to change the versions checks for that feature then we can easily change it.

radical · 2023-07-07T04:13:55Z

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

+            AssemblyAndPdbData ret = new(assembly_buf, pdb_buf);
+
+            if (MajorVersion == 2 && MinorVersion >= 64)


nit:

if (MajorVersion != 2 || MinorVersion < 64) return new(assembly_buf, pdb_buf); AssemblyAndPdbData ret = new()

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

Co-authored-by: Ankit Jain <radical@gmail.com>

…sed_on_jmc

radical · 2023-07-13T20:09:34Z

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

@@ -1909,7 +1944,7 @@ public async Task<string> InvokeToStringAsync(IEnumerable<int> typeIds, bool isV
                    foreach (var methodId in methodIds)
                    {
                        var methodInfoFromRuntime = await GetMethodInfo(methodId, token);
-                        if (methodInfoFromRuntime.Info.GetParametersInfo().Length > 0)
+                        if (methodInfoFromRuntime?.Info?.GetParametersInfo()?.Length > 0)


Should we also check that the return type is string?
And this method (InvokeTostringAsync) is only looking at instance methods, correct?

src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs

…hub.com:thaystg/runtime into thays_improve_debugger_performance_based_on_jmc

radical

LGTM! 👍

Improve debugger performance.

58562fb

dotnet-issue-labeler bot added the area-Debugger-mono label May 31, 2023

ghost assigned thaystg May 31, 2023

thaystg added the arch-wasm WebAssembly architecture label May 31, 2023

Loading assembly bytes if JMC is disabled after the debugger session …

ef3da1f

…is started.

thaystg marked this pull request as ready for review June 1, 2023 19:11

thaystg requested a review from radical as a code owner June 1, 2023 19:11

thaystg requested a review from lambdageek June 1, 2023 19:11

Fix CI.

1027969

thaystg marked this pull request as draft June 1, 2023 20:36

Load symbols on demand if JMC is enabled, we don't need to spend a lo…

6084272

…t of time loading information from 149 assemblies if we will probably not need all of them.

thaystg marked this pull request as ready for review June 2, 2023 14:08

ilonatommy approved these changes Jun 20, 2023

View reviewed changes

Merge branch 'dotnet:main' into thays_improve_debugger_performance_ba…

38ad5f7

…sed_on_jmc

build-analysis bot mentioned this pull request Jun 26, 2023

[mono][dbg] EvaluateConstantValueUsingRuntimeEvaluate test crash on Chrome #87053

Closed

Impriving the performance sending only metadata and not the full asse…

898bdfc

…mbly.

thaystg requested a review from marek-safar as a code owner June 28, 2023 16:06

thaystg requested a review from ilonatommy June 28, 2023 16:06

Fixing compilation error on tvos

7565f63

radical reviewed Jun 28, 2023

View reviewed changes

src/mono/mono/component/debugger-agent.c Show resolved Hide resolved

build-analysis bot mentioned this pull request Jun 28, 2023

Tracking issue for CI build timeouts #76454

Closed

ilonatommy approved these changes Jun 29, 2023

View reviewed changes

radical reviewed Jul 7, 2023

View reviewed changes

thaystg and others added 2 commits July 10, 2023 11:23

Apply suggestions from code review

baf3c84

Co-authored-by: Ankit Jain <radical@gmail.com>

Apply suggestions from code review

6222b1e

Co-authored-by: Ankit Jain <radical@gmail.com>

thaystg added 2 commits July 10, 2023 11:48

fix changes from code review

d50bfb4

addressing @radical comments

7b59456

build-analysis bot mentioned this pull request Jul 10, 2023

[wasm] Assertion failure in DebuggerTests.HotReloadTests on main - src/mono/mono/component/debugger-engine.c:176 #88528

Closed

thaystg requested a review from radical July 11, 2023 18:03

Merge branch 'dotnet:main' into thays_improve_debugger_performance_ba…

8b7c51a

…sed_on_jmc

radical reviewed Jul 13, 2023

View reviewed changes

radical requested changes Jul 13, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 13, 2023

thaystg added 3 commits July 13, 2023 23:47

addressing @radical and @lewing comments

e183804

Merge branch 'main' into thays_improve_debugger_performance_based_on_jmc

c16349e

Merge branch 'thays_improve_debugger_performance_based_on_jmc' of git…

6becf9c

…hub.com:thaystg/runtime into thays_improve_debugger_performance_based_on_jmc

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 14, 2023

radical approved these changes Jul 14, 2023

View reviewed changes

This was referenced Jul 14, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

thaystg merged commit ae99bb2 into dotnet:main Jul 14, 2023

thaystg mentioned this pull request Jul 17, 2023

[wasm][debugger] Check client version to send debugger message #89025

Merged

thaystg mentioned this pull request Aug 5, 2023

[Blazor] Slow startup and navigation when debugging #80446

Closed

1 task

ghost locked as resolved and limited conversation to collaborators Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm][debugger] Improve debugger performance based on JMC #86982

[wasm][debugger] Improve debugger performance based on JMC #86982

thaystg commented May 31, 2023 •

edited

Loading

ghost commented May 31, 2023

thaystg commented May 31, 2023

ghost commented May 31, 2023

lewing commented May 31, 2023

thaystg commented Jun 1, 2023 •

edited

Loading

ilonatommy Jun 20, 2023

radical Jul 7, 2023

ilonatommy left a comment

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 7, 2023

radical Jul 13, 2023 •

edited

Loading

radical left a comment

		{
		if (!assemblyName.StartsWith("System.Private.CoreLib") && !ignoreJMC && proxy.JustMyCode && !(await HasDebugInfoLoadedByRuntime(assemblyName, token)))

		AssemblyAndPdbData ret = new(assembly_buf, pdb_buf);

		if (MajorVersion == 2 && MinorVersion >= 64)

[wasm][debugger] Improve debugger performance based on JMC #86982

[wasm][debugger] Improve debugger performance based on JMC #86982

Conversation

thaystg commented May 31, 2023 • edited Loading

ghost commented May 31, 2023

thaystg commented May 31, 2023

ghost commented May 31, 2023

lewing commented May 31, 2023

thaystg commented Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilonatommy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radical Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

radical left a comment

Choose a reason for hiding this comment

thaystg commented May 31, 2023 •

edited

Loading

thaystg commented Jun 1, 2023 •

edited

Loading

radical Jul 13, 2023 •

edited

Loading