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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Nov 16, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
72ad108
Fix memory consumption.
thaystg Nov 11, 2021
d65373b
Merge branch 'dotnet:main' into thays_fix_memory_evaluation
thaystg Nov 11, 2021
9755954
Fix debugger-tests
thaystg Nov 11, 2021
3e832bf
Fix compilation.
thaystg Nov 11, 2021
498e2c1
Addressing @lewing PR.
thaystg Nov 11, 2021
d505d40
Address @lewing comment
thaystg Nov 11, 2021
69ad647
Addressing @radical comment.
thaystg Nov 11, 2021
d8e3e57
Addressing comments.
thaystg Nov 11, 2021
0128938
Addressing @radical comments.
thaystg Nov 11, 2021
194ee4f
missing return.
thaystg Nov 11, 2021
36e7e56
Addressing @radical comments
thaystg Nov 11, 2021
0a686ec
Adding test case
thaystg Nov 11, 2021
97b98ce
Fixing tests.
thaystg Nov 11, 2021
bd552fd
Adding another test case. Thanks @lewing.
thaystg Nov 11, 2021
351f897
Reuse the script.
thaystg Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/mono/wasm/debugger/BrowserDebugProxy/BrowserDebugProxy.csproj
View file
Open in desktop
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
198 changes: 74 additions & 124 deletions src/mono/wasm/debugger/BrowserDebugProxy/EvaluateExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,28 @@
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;
using System.Text.RegularExpressions;

namespace Microsoft.WebAssembly.Diagnostics
{
internal static class EvaluateExpression
{
internal static Script<object> script = CSharpScript.Create(
"",
ScriptOptions.Default.WithReferences(
typeof(object).Assembly,
typeof(Enumerable).Assembly,
typeof(JObject).Assembly
));
private class FindVariableNMethodCall : CSharpSyntaxWalker
{
private static Regex regexForReplaceVarName = new Regex(@"[^A-Za-z0-9_]", RegexOptions.Singleline);
public List<IdentifierNameSyntax> identifiers = new List<IdentifierNameSyntax>();
public List<InvocationExpressionSyntax> methodCall = new List<InvocationExpressionSyntax>();
public List<MemberAccessExpressionSyntax> memberAccesses = new List<MemberAccessExpressionSyntax>();
Expand All @@ -32,6 +43,7 @@ private class FindVariableNMethodCall : CSharpSyntaxWalker
private int visitCount;
public bool hasMethodCalls;
public bool hasElementAccesses;
internal List<string> variableDefinitions = new List<string>();

public void VisitInternal(SyntaxNode node)
{
Expand Down Expand Up @@ -97,7 +109,7 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
{
// Generate a random suffix
string suffix = Guid.NewGuid().ToString().Substring(0, 5);
string prefix = ma_str.Trim().Replace(".", "_");
string prefix = regexForReplaceVarName.Replace(ma_str, "_");
id_name = $"{prefix}_{suffix}";

memberAccessToParamName[ma_str] = id_name;
Expand All @@ -114,7 +126,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 = regexForReplaceVarName.Replace(iesStr, "_");
id_name = $"{prefix}_{suffix}";
methodCallToParamName[iesStr] = id_name;
}
Expand All @@ -138,7 +150,7 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
return SyntaxFactory.IdentifierName(id_name);
});

var paramsSet = new HashSet<string>();
var localsSet = new HashSet<string>();

// 2. For every unique member ref, add a corresponding method param
if (ma_values != null)
Expand All @@ -151,15 +163,15 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
throw new Exception($"BUG: Expected to find an id name for the member access string: {node_str}");
}
memberAccessValues[id_name] = value;
root = UpdateWithNewMethodParam(root, id_name, value);
AddLocalVariableWithValue(id_name, value);
}
}

if (id_values != null)
{
foreach ((IdentifierNameSyntax idns, JObject value) in identifiers.Zip(id_values))
{
root = UpdateWithNewMethodParam(root, idns.Identifier.Text, value);
AddLocalVariableWithValue(idns.Identifier.Text, value);
}
}

Expand All @@ -172,7 +184,7 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
{
throw new Exception($"BUG: Expected to find an id name for the member access string: {node_str}");
}
root = UpdateWithNewMethodParam(root, id_name, value);
AddLocalVariableWithValue(id_name, value);
}
}

Expand All @@ -185,96 +197,73 @@ public SyntaxTree ReplaceVars(SyntaxTree syntaxTree, IEnumerable<JObject> ma_val
{
throw new Exception($"BUG: Expected to find an id name for the element access string: {node_str}");
}
root = UpdateWithNewMethodParam(root, id_name, value);
AddLocalVariableWithValue(id_name, value);
}
}

return syntaxTree.WithRootAndOptions(root, syntaxTree.Options);

CompilationUnitSyntax UpdateWithNewMethodParam(CompilationUnitSyntax root, string id_name, JObject value)
void AddLocalVariableWithValue(string idName, 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);

return root;
if (localsSet.Contains(idName))
return;
localsSet.Add(idName);
variableDefinitions.Add(ConvertJSToCSharpLocalVariableAssignment(idName, value));
}
}

private object ConvertJSToCSharpType(JToken variable)
private string ConvertJSToCSharpLocalVariableAssignment(string idName, JToken variable)
{
string typeRet;
object valueRet;
JToken value = variable["value"];
string type = variable["type"].Value<string>();
string subType = variable["subtype"]?.Value<string>();

string objectId = variable["objectId"]?.Value<string>();
switch (type)
radical marked this conversation as resolved.
Show resolved Hide resolved
{
case "string":
return value?.Value<string>();
{
var str = value?.Value<string>();
str = str.Replace("\"", "\\\"");
valueRet = $"\"{str}\"";
typeRet = "string";
break;
}
case "number":
return value?.Value<double>();
valueRet = value?.Value<double>();
typeRet = "double";
break;
case "boolean":
return value?.Value<bool>();
valueRet = value?.Value<string>().ToLower();
typeRet = "bool";
break;
case "object":
return variable;
valueRet = "Newtonsoft.Json.Linq.JObject.FromObject(new {"
+ $"type = \"{type}\""
+ $", description = \"{variable["description"].Value<string>()}\""
+ $", className = \"{variable["className"].Value<string>()}\""
+ (subType != null ? $", subtype = \"{subType}\"" : "")
+ (objectId != null ? $", objectId = \"{objectId}\"" : "")
+ "})";
typeRet = "object";
break;
case "void":
return null;
}
throw new Exception($"Evaluate of this datatype {type} not implemented yet");//, "Unsupported");
}

private string GetTypeFullName(JToken variable)
{
string type = variable["type"].ToString();
string subType = variable["subtype"]?.Value<string>();
object value = ConvertJSToCSharpType(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";
}
case "void":
return "object";
valueRet = "Newtonsoft.Json.Linq.JObject.FromObject(new {"
+ $"type = \"object\","
+ $"description = \"object\","
+ $"className = \"object\","
+ $"subtype = \"null\""
+ "})";
typeRet = "object";
break;
Comment on lines +242 to +259
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand these two.

  1. why do we need to use JObjects in the script?
  2. case "object": - would it be better to use Newtonsoft.Json.Linq.JObject.Parse(..) and pass the escaped string for variable?
  3. case "void": - why is this needed? And what does the variable look like here? And do we have a test for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Because that is how we return simple variable values, so when it's something that we really need to execute we also keep the same way of returning the value.
  2. I think it's faster to not parse the string? once this is already parsed we don't need to parse it again?
  3. Yes, the tests evaluating method that return void.

default:
return value.GetType().FullName;
throw new Exception($"Evaluate of this datatype {type} not implemented yet");//, "Unsupported");
}
throw new ReturnAsErrorException($"GetTypefullName: Evaluate of this datatype {type} not implemented yet", "Unsupported");
return $"{typeRet} {idName} = {valueRet};";
}
}

private static SyntaxNode GetExpressionFromSyntaxTree(SyntaxTree syntaxTree)
{
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;
}

private static async Task<IList<JObject>> ResolveMemberAccessExpressions(IEnumerable<MemberAccessExpressionSyntax> member_accesses,
MemberReferenceResolver resolver, CancellationToken token)
{
Expand Down Expand Up @@ -335,32 +324,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);
SyntaxNode expressionTree = syntaxTree.GetCompilationUnitRoot(token);
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 @@ -388,7 +365,7 @@ public static object Evaluate()

if (findVarNMethodCall.hasMethodCalls)
{
expressionTree = GetExpressionFromSyntaxTree(syntaxTree);
expressionTree = syntaxTree.GetCompilationUnitRoot(token);

findVarNMethodCall.VisitInternal(expressionTree);

Expand All @@ -400,7 +377,7 @@ public static object Evaluate()
// eg. "elements[0]"
if (findVarNMethodCall.hasElementAccesses)
{
expressionTree = GetExpressionFromSyntaxTree(syntaxTree);
expressionTree = syntaxTree.GetCompilationUnitRoot(token);

findVarNMethodCall.VisitInternal(expressionTree);

Expand All @@ -409,48 +386,21 @@ public static object Evaluate()
syntaxTree = findVarNMethodCall.ReplaceVars(syntaxTree, null, null, null, elementAccessValues);
}

expressionTree = GetExpressionFromSyntaxTree(syntaxTree);
expressionTree = syntaxTree.GetCompilationUnitRoot(token);
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));
try {
var newScript = script.ContinueWith(
radical marked this conversation as resolved.
Show resolved Hide resolved
string.Join("\n", findVarNMethodCall.variableDefinitions) + "\nreturn " + syntaxTree.ToString());

SemanticModel semanticModel = compilation.GetSemanticModel(syntaxTree);
CodeAnalysis.TypeInfo TypeInfo = semanticModel.GetTypeInfo(expressionTree, cancellationToken: token);
var state = await newScript.RunAsync(cancellationToken: token);

using (var ms = new MemoryStream())
return JObject.FromObject(ConvertCSharpToJSType(state.ReturnValue, state.ReturnValue.GetType()));
radical marked this conversation as resolved.
Show resolved Hide resolved
}
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 +412,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
7 changes: 6 additions & 1 deletion src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs
View file
Open in desktop
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 Expand Up @@ -542,6 +542,11 @@ await EvaluateOnCallFrameAndCheck(id,
("this.CallMethodWithParmBool(true)", TString("TRUE")),
("this.CallMethodWithParmBool(false)", TString("FALSE")),
("this.CallMethodWithParmString(\"concat\")", TString("str_const_concat")),
("this.CallMethodWithParmString(\"\\\"\\\"\")", TString("str_const_\"\"")),
thaystg marked this conversation as resolved.
Show resolved Hide resolved
("this.CallMethodWithParmString(\"🛶\")", TString("str_const_🛶")),
("this.CallMethodWithParmString(\"\\uD83D\\uDEF6\")", TString("str_const_🛶")),
("this.CallMethodWithParmString(\"🚀\")", TString("str_const_🚀")),
("this.CallMethodWithParmString_λ(\"🚀\")", TString("λ_🚀")),
("this.CallMethodWithParm(10) + this.a", TNumber(12)),
("this.CallMethodWithObj(null)", TNumber(-1)),
("this.CallMethodWithChar('a')", TString("str_const_a")));
Expand Down
5 changes: 5 additions & 0 deletions src/mono/wasm/debugger/tests/debugger-test/debugger-evaluate-test.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,11 @@ public string CallMethodWithParmString(string parm)
return str + parm;
}

public string CallMethodWithParmString_λ(string parm)
{
return "λ_" + parm;
}

public string CallMethodWithParmBool(bool parm)
{
if (parm)
Expand Down