Skip to content
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

[wasm][debugger] Improvement of memory consumption while Evaluating Expressions #61470

Merged
merged 15 commits into from
Nov 16, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="3.7.0" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future PR, we should consider using the common version (more than one!) property for this, being used in other places in the repo.

<PackageReference Include="Microsoft.Extensions.Logging" Version="3.1.7" />
<PackageReference Include="Newtonsoft.Json" Version="12.0.3" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.7.0" />
Expand Down
129 changes: 47 additions & 82 deletions src/mono/wasm/debugger/BrowserDebugProxy/EvaluateExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Scripting;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Emit;
using Microsoft.CodeAnalysis.Scripting;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

Expand All @@ -32,6 +34,7 @@ private class FindVariableNMethodCall : CSharpSyntaxWalker
private int visitCount;
public bool hasMethodCalls;
public bool hasElementAccesses;
internal string variableDefinition;
radical marked this conversation as resolved.
Show resolved Hide resolved

public void VisitInternal(SyntaxNode node)
{
Expand Down Expand Up @@ -114,7 +117,7 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
{
// Generate a random suffix
string suffix = Guid.NewGuid().ToString().Substring(0, 5);
string prefix = iesStr.Trim().Replace(".", "_").Replace("(", "_").Replace(")", "_");
string prefix = iesStr.Trim().Replace(".", "_").Replace("(", "_").Replace(")", "_").Replace(", ", "_").Replace("\"", "_").Replace("'", "_");
lewing marked this conversation as resolved.
Show resolved Hide resolved
id_name = $"{prefix}_{suffix}";
methodCallToParamName[iesStr] = id_name;
}
Expand Down Expand Up @@ -193,26 +196,14 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val

CompilationUnitSyntax UpdateWithNewMethodParam(CompilationUnitSyntax root, string id_name, JObject value)
{
var classDeclaration = root.Members.ElementAt(0) as ClassDeclarationSyntax;
var method = classDeclaration.Members.ElementAt(0) as MethodDeclarationSyntax;

if (paramsSet.Contains(id_name))
{
// repeated member access expression
// eg. this.a + this.a
return root;
}

argValues.Add(ConvertJSToCSharpType(value));

MethodDeclarationSyntax updatedMethod = method.AddParameterListParameters(
SyntaxFactory.Parameter(
SyntaxFactory.Identifier(id_name))
.WithType(SyntaxFactory.ParseTypeName(GetTypeFullName(value))));

paramsSet.Add(id_name);
root = root.ReplaceNode(method, updatedMethod);

variableDefinition += $"{GetTypeFullName(value)} {id_name} = {ConvertJSToCSharpType(value)};\n";
return root;
}
}
Expand All @@ -222,19 +213,37 @@ private object ConvertJSToCSharpType(JToken variable)
JToken value = variable["value"];
string type = variable["type"].Value<string>();
string subType = variable["subtype"]?.Value<string>();

switch (type)
radical marked this conversation as resolved.
Show resolved Hide resolved
{
case "string":
return value?.Value<string>();
return $"\"{value?.Value<string>()}\"";
radical marked this conversation as resolved.
Show resolved Hide resolved
case "number":
return value?.Value<double>();
case "boolean":
return value?.Value<bool>();
return value?.Value<string>().ToLower();
case "object":
return variable;
{
if (subType == "null")
lewing marked this conversation as resolved.
Show resolved Hide resolved
return "Newtonsoft.Json.Linq.JObject.FromObject(new {"
+ $"type = \"{type}\","
+ $"description = \"{variable["description"].Value<string>()}\","
+ $"className = \"{variable["className"].Value<string>()}\","
+ $"subtype = \"{subType}\""
+ "})";
return "Newtonsoft.Json.Linq.JObject.FromObject(new {"
+ $"type = \"{type}\","
+ $"description = \"{variable["description"].Value<string>()}\","
+ $"className = \"{variable["className"].Value<string>()}\","
+ $"objectId = \"{variable["objectId"].Value<string>()}\""
+ "})";
}
case "void":
return null;
return "Newtonsoft.Json.Linq.JObject.FromObject(new {"
+ $"type = \"object\","
+ $"description = \"object\","
+ $"className = \"object\","
+ $"subtype = \"null\""
+ "})";
}
throw new Exception($"Evaluate of this datatype {type} not implemented yet");//, "Unsupported");
}
Expand All @@ -248,14 +257,11 @@ private string GetTypeFullName(JToken variable)
switch (type)
{
case "object":
{
if (subType == "null")
lewing marked this conversation as resolved.
Show resolved Hide resolved
return variable["className"].Value<string>();
else
return "object";
}
return "object";
case "void":
return "object";
case "boolean":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this case to the tests too

Copy link
Member Author

@thaystg thaystg Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is testing it

("this.CallMethodWithParmBool(this.t)", TString("TRUE")),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and this was being handled by default: return value.GetType().FullName; earlier? Why the additional case then?

Copy link
Member Author

@thaystg thaystg Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I did this change:
image

And this change was needed because when the variable was being declared and used it was creating like this: var exampleVar4893742 = True; and the first letter of the True/False with uppercase throws a compilation error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that makes sense. IIUC, ConvertJSToCSharpType is being called once to get the value, and then GetTypeFullName also calls it. Instead, I think it would make sense for ConvertJSToCSharpType to return a (string type, string value), since the type that we want to use in the final expression is tied closely to the "value" that we return.

And the method can be renamed to something like ConvertJSToCSharpVariable.

return "System.Boolean";
default:
return value.GetType().FullName;
}
Expand All @@ -266,13 +272,8 @@ private string GetTypeFullName(JToken variable)
private static SyntaxNode GetExpressionFromSyntaxTree(SyntaxTree syntaxTree)
lewing marked this conversation as resolved.
Show resolved Hide resolved
{
CompilationUnitSyntax root = syntaxTree.GetCompilationUnitRoot();
ClassDeclarationSyntax classDeclaration = root.Members.ElementAt(0) as ClassDeclarationSyntax;
MethodDeclarationSyntax methodDeclaration = classDeclaration.Members.ElementAt(0) as MethodDeclarationSyntax;
BlockSyntax blockValue = methodDeclaration.Body;
ReturnStatementSyntax returnValue = blockValue.Statements.ElementAt(0) as ReturnStatementSyntax;
ParenthesizedExpressionSyntax expressionParenthesized = returnValue.Expression as ParenthesizedExpressionSyntax;

return expressionParenthesized?.Expression;
return root;
}

private static async Task<IList<JObject>> ResolveMemberAccessExpressions(IEnumerable<MemberAccessExpressionSyntax> member_accesses,
Expand Down Expand Up @@ -335,32 +336,20 @@ private static async Task<IList<JObject>> ResolveElementAccess(IEnumerable<Eleme
return values;
}

[UnconditionalSuppressMessage("SingleFile", "IL3000:Avoid accessing Assembly file path when publishing as a single file",
Justification = "Suppressing the warning until gets fixed, see https://github.com/dotnet/runtime/issues/51202")]
internal static async Task<JObject> CompileAndRunTheExpression(string expression, MemberReferenceResolver resolver, CancellationToken token)
{
expression = expression.Trim();
if (!expression.StartsWith('('))
{
expression = "(" + expression + ")";
}
SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(@"
using System;
public class CompileAndRunTheExpression
{
public static object Evaluate()
{
return " + expression + @";
}
}", cancellationToken: token);
SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(expression + @";", cancellationToken: token);

SyntaxNode expressionTree = GetExpressionFromSyntaxTree(syntaxTree);
if (expressionTree == null)
throw new Exception($"BUG: Unable to evaluate {expression}, could not get expression from the syntax tree");

FindVariableNMethodCall findVarNMethodCall = new FindVariableNMethodCall();
findVarNMethodCall.VisitInternal(expressionTree);

// this fails with `"a)"`
// because the code becomes: return (a));
// and the returned expression from GetExpressionFromSyntaxTree is `a`!
Expand Down Expand Up @@ -413,44 +402,20 @@ public static object Evaluate()
if (expressionTree == null)
throw new Exception($"BUG: Unable to evaluate {expression}, could not get expression from the syntax tree");

MetadataReference[] references = new MetadataReference[]
{
MetadataReference.CreateFromFile(typeof(object).Assembly.Location),
MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location)
};

CSharpCompilation compilation = CSharpCompilation.Create(
"compileAndRunTheExpression",
syntaxTrees: new[] { syntaxTree },
references: references,
options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));

SemanticModel semanticModel = compilation.GetSemanticModel(syntaxTree);
CodeAnalysis.TypeInfo TypeInfo = semanticModel.GetTypeInfo(expressionTree, cancellationToken: token);

using (var ms = new MemoryStream())
try {
var task = CSharpScript.EvaluateAsync(
radical marked this conversation as resolved.
Show resolved Hide resolved
findVarNMethodCall.variableDefinition + "return " + syntaxTree.ToString(),
ScriptOptions.Default.WithReferences(
typeof(object).Assembly,
typeof(Enumerable).Assembly,
typeof(JObject).Assembly
),
cancellationToken: token);
return JObject.FromObject(ConvertCSharpToJSType(task.Result, task.Result.GetType()));
}
catch (Exception)
{
EmitResult result = compilation.Emit(ms, cancellationToken: token);
if (!result.Success)
{
var sb = new StringBuilder();
foreach (Diagnostic d in result.Diagnostics)
sb.Append(d.ToString());

throw new ReturnAsErrorException(sb.ToString(), "CompilationError");
}

ms.Seek(0, SeekOrigin.Begin);
Assembly assembly = Assembly.Load(ms.ToArray());
Type type = assembly.GetType("CompileAndRunTheExpression");

object ret = type.InvokeMember("Evaluate",
BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public,
null,
null,
findVarNMethodCall.argValues.ToArray());

return JObject.FromObject(ConvertCSharpToJSType(ret, TypeInfo.Type));
throw new ReturnAsErrorException($"Cannot evaluate '{expression}'.", "CompilationError");
}
}

Expand All @@ -462,7 +427,7 @@ public static object Evaluate()
typeof(float), typeof(double)
};

private static object ConvertCSharpToJSType(object v, ITypeSymbol type)
private static object ConvertCSharpToJSType(object v, Type type)
{
if (v == null)
return new { type = "object", subtype = "null", className = type.ToString(), description = type.ToString() };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ public async Task NegativeTestsInStaticMethod() => await CheckInspectLocalsAtBre

await EvaluateOnCallFrameFail(id,
("me.foo", "ReferenceError"),
("this", "ReferenceError"),
("this", "CompilationError"),
("this.NullIfAIsNotZero.foo", "ReferenceError"));
});

Expand Down